Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Used np.divide with out and where parameters #5501

Merged
merged 4 commits into from
Jan 23, 2025

Conversation

SonaliBedge
Copy link
Contributor

@SonaliBedge SonaliBedge commented Jan 22, 2025

Fixes issue #4888 (Used np.divide NumPy function with out and where parameters to solve invalid value encountered when dividing employment_income by total_earnings in marginal tax rate calculation.)

@MaxGhenis
Copy link
Contributor

MaxGhenis commented Jan 22, 2025

Thanks @SonaliBedge! Could you please:

  1. Connect this to an issue. If it's not connected to one, please create the issue where you describe the problem (we always tie PRs to issues).
  2. Remove the commented code
  3. Populate changelog_entry.yaml (see other PRs)
  4. Run make format
  5. File an issue to change all mask operations for avoiding divide-by-zeros to np.divide using this approach, as it's more concise, readable, and performant

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.04%. Comparing base (cb92a56) to head (26ba5f3).
Report is 369 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5501      +/-   ##
==========================================
- Coverage   99.12%   99.04%   -0.08%     
==========================================
  Files        2592     2678      +86     
  Lines       37707    38807    +1100     
  Branches      162      168       +6     
==========================================
+ Hits        37378    38438    +1060     
- Misses        297      335      +38     
- Partials       32       34       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SonaliBedge
Copy link
Contributor Author

Thanks @SonaliBedge! Could you please:

  1. Connect this to an issue. If it's not connected to one, please create the issue where you describe the problem (we always tie PRs to issues).
  2. Remove the commented code
  3. Populate changelog_entry.yaml (see other PRs)
  4. Run make format
  5. File an issue to change all mask operations for avoiding divide-by-zeros to np.divide using this approach, as it's more concise, readable, and performant

Thank you, Max, for your comments. I will make all the above changes. This is my first Policy Engine PR, and I wasn’t fully familiar with the process, which is why I didn’t remove the commented code.

@MaxGhenis
Copy link
Contributor

Populate changelog_entry.yaml not changelog.yaml

changelog_entry.yaml Outdated Show resolved Hide resolved
changelog_entry.yaml Outdated Show resolved Hide resolved
changelog_entry.yaml Outdated Show resolved Hide resolved
@MaxGhenis MaxGhenis merged commit 89624f0 into PolicyEngine:master Jan 23, 2025
4 of 5 checks passed
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.

2 participants