Skip to content

Conversation

@nikhilwoodruff
Copy link

No description provided.

Copy link
Contributor

@vahid-ahmadi vahid-ahmadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Thanks for this comprehensive guide! I found a few critical issues that should be fixed before merging:

Critical Issues:

  1. Line 3: Typo - double space in "PolicyEngine UK ONLY"
  2. Line 454: Incorrect parameter - map_to="person" when age is already person-level
  3. Line 774: Formatting - missing newline before section header

I'll add inline comments on these specific lines.

Copy link
Contributor

@vahid-ahmadi vahid-ahmadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 3: Typo - there's a double space between "PolicyEngine" and "UK". Should be:

Guide for implementing policy reforms and microsimulation analysis in PolicyEngine UK ONLY projects.

Copy link
Contributor

@vahid-ahmadi vahid-ahmadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 454: Incorrect parameter usage. The code has:

age = sim.calculate("age", map_to="person")

Since age is already a person-level variable, you don't need map_to="person". This line should either be:

age = sim.calculate("age")  # Already person-level

Or if you're trying to aggregate to household level, it should be:

age = sim.calculate("age", map_to="household")

Copy link
Contributor

@vahid-ahmadi vahid-ahmadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 774: Formatting issue - missing newline before section header. There should be a blank line between the closing of the previous code block and the "### Conditional reform application" header.

Current:

    sim.tax_benefit_system.process_parameters()

Conditional reform application

Should be:

    sim.tax_benefit_system.process_parameters()

Conditional reform application

Copy link
Contributor

@vahid-ahmadi vahid-ahmadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Note: There are several incomplete code examples that should be filled in:

  • Line 328: # Use the values... leaves the example incomplete
  • Line 352: # Apply logic based on working_count... is truncated
  • Line 796: # ... complex logic here should show actual implementation

These placeholder comments should either be completed with actual code or removed if they're meant to be filled in by the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants