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

Compute tax-scale bracket index and marginal rate #920

Merged
merged 15 commits into from
Dec 31, 2019
Merged

Conversation

benjello
Copy link
Member

@benjello benjello commented Dec 24, 2019

New features

  • Introduce AbstractTaxScale.bracket_indices

  • Allows for computing the bracket index relevant for any taxable base vector

  • Introduce MarginalTaxScale.marginal_rates

  • Allows for computing the marginal rate relevant for any taxable base vector

see also openfisca/openfisca-france#1386

@bonjourmauko
Copy link
Member

Thanks @benjello !

For this particular pull request, tests aren't enough for me to grasp the intent and the behaviours introduced by it.

Specifically, it'd be great to:

  • Add a description with an example to each new method
  • Explain the difference between calc and compute

That'll help me a lot to review your contribution! :)

@bonjourmauko
Copy link
Member

Rebase + fixup

@bonjourmauko bonjourmauko force-pushed the enhance-tax-scale branch 2 times, most recently from e1632ef to 89c0d85 Compare December 27, 2019 16:45
openfisca_core/taxscales.py Outdated Show resolved Hide resolved
@bonjourmauko
Copy link
Member

Hey @benjello I profited to add some documentation and typing to better understand the behaviours of these classes, before doing a review of the proposed contribution.

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Thanks a lot @benjello, I added some tests with my guesses and there are still two that fail (see in the comments).

Could you take a look?

tests/core/tax_scales/test_abstract_rate_tax_scale.py Outdated Show resolved Hide resolved
tests/core/tax_scales/test_abstract_rate_tax_scale.py Outdated Show resolved Hide resolved
@bonjourmauko bonjourmauko force-pushed the enhance-tax-scale branch 3 times, most recently from 5e8c580 to 20bf838 Compare December 31, 2019 19:34
@bonjourmauko
Copy link
Member

bonjourmauko commented Dec 31, 2019

I profited of the opportunity to add a user-friendly message:

Capture d’écran 2019-12-31 à 20 37 15

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

LGTM ✨

@bonjourmauko bonjourmauko requested a review from a team December 31, 2019 20:12
@benjello benjello merged commit 871bf20 into master Dec 31, 2019
@bonjourmauko bonjourmauko deleted the enhance-tax-scale branch March 31, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat A feature request, a feature deprecation level:intermediate Requires average OpenFisca experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants