Skip to content

Conversation

@nmrodelo
Copy link
Contributor

Draft/ work in progress pull request for snap minimum benefit parameters

Copy link
Contributor

@MaxGhenis MaxGhenis left a comment

Choose a reason for hiding this comment

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

I know just a draft but a couple comments after checking it out

@MaxGhenis MaxGhenis changed the title add snap minimum benefit params Add SNAP minimum benefit Dec 4, 2021
@nmrodelo nmrodelo self-assigned this Dec 4, 2021
@MaxGhenis
Copy link
Contributor

I'm not sure why relevant_max_benefit is null in the calculation. @nikhilwoodruff any ideas?

https://github.com/PolicyEngine/openfisca-us/runs/4419215730?check_suite_focus=true#step:5:591

@nmrodelo
Copy link
Contributor Author

@nikhilwoodruff could you take a look at this?

Copy link
Collaborator

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

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

Just took a look @nmrodelo - it's what I think is a bug in Core (I've seen it before in a few places). Essentially, when we index a parameter with an array of numbers (e.g. amount_by_household_size[[1, 1, 1, 3, 1, 2]]) we get the right values, but when we index by just a single number (amount_by_household_size[1]) it doesn't seem to work - except when we do amount_by_household_size["1"]. Here's how I fixed it locally below.

Co-authored-by: Nikhil Woodruff <35577657+nikhilwoodruff@users.noreply.github.com>
@nmrodelo
Copy link
Contributor Author

@nikhilwoodruff is there anything left on my end for this PR?

@MaxGhenis
Copy link
Contributor

Try renaming snap_min_benefit to snap_minimum_benefit in snap.py

Copy link
Contributor

@MaxGhenis MaxGhenis left a comment

Choose a reason for hiding this comment

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

Please also rename the test file to snap_minimum_benefit.yaml to match the variable name.

@codecov
Copy link

codecov bot commented Dec 25, 2021

Codecov Report

Merging #315 (d1b7e8b) into master (39a24df) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #315   +/-   ##
=======================================
  Coverage   94.87%   94.88%           
=======================================
  Files          59       59           
  Lines        3652     3654    +2     
  Branches      480      480           
=======================================
+ Hits         3465     3467    +2     
  Misses        162      162           
  Partials       25       25           
Impacted Files Coverage Δ
openfisca_us/variables/usda/snap/snap.py 100.00% <ø> (ø)
openfisca_us/variables/usda/snap/min_benefit.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39a24df...d1b7e8b. Read the comment docs.

@MaxGhenis
Copy link
Contributor

Thanks @nmrodelo, could you just do two more things? We're trying to get back on the http://semver.org track. You'll have to pull to fetch my latest PR.

  1. Update the version to 0.4.0 in setup.py (this PR adds new functionality, so it's a minor change rather than patch)
  2. Update CHANGELOG.md with an entry for 0.4.0 saying Add SNAP minimum benefit

I'll merge from there.

Copy link
Collaborator

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @nmrodelo. The semantic versioning GH action now looks like it's working (see the error message) - could you just add to the CHANGELOG.md (referencing this #315 PR like the openfisca one)? Looks good to merge straight after that

@nikhilwoodruff
Copy link
Collaborator

@nmrodelo thanks for this PR! Just added the changelog entry, merging.

@nikhilwoodruff nikhilwoodruff merged commit a7da21f into PolicyEngine:master Dec 28, 2021
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