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

Parameterize the limitation on capital losses #2707

Merged
merged 7 commits into from
Jan 29, 2024

Conversation

jdebacker
Copy link
Member

This PR addresses Issue #2632 by parameterizing the capital loss limitation.

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #2707 (bf6332a) into master (6798fba) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2707   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files          14       14           
  Lines        2609     2609           
=======================================
  Hits         2571     2571           
  Misses         38       38           
Flag Coverage Δ
unittests 98.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
taxcalc/utils.py 97.79% <ø> (ø)

@jdebacker
Copy link
Member Author

@MattHJensen This PR is ready for your review. Thanks!

@MattHJensen
Copy link
Contributor

@jdebacker, do you think we should protect puf and cps users from setting this parameter outside of the ranges supported by their data? Or say this param isn't compatible with those data?

@jdebacker
Copy link
Member Author

@MattHJensen Good point about data limitations affecting this parameter's usefulness. To address this concern, I've added a note about this in the "notes" metadata field for the capital_loss_limitation parameter.

Note that I am reluctant to set "puf": false in the compatible_data field because the parameter will work with the PUF as expected, so long as the values is less than or equal to the current law value (as the PUF does not provide information on capital losses beyond the current law limit).

Do you think this is satisfactory?

@jdebacker
Copy link
Member Author

@MattHJensen I think this PR is ready to merge. Let me know if you have further thoughts. Will merge if I don't hear back in about 24hrs. Thanks!

@jdebacker jdebacker merged commit 070c82e into PSLmodels:master Jan 29, 2024
11 checks passed
@jdebacker jdebacker deleted the cap branch January 29, 2024 18:20
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