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

Improve housing tax and add metadata to parameters #49

Merged
merged 9 commits into from
Jul 16, 2018
Merged

Conversation

fpagnoux
Copy link
Member

@fpagnoux fpagnoux commented Jul 6, 2018

Connected to openfisca/openfisca-core#677
Supersedes #48

This PR aims at setting good practice for parameters metadata, pay attention to this part :)

@fpagnoux fpagnoux changed the title Parse metadata Improve housing tax and add metadata to parameters Jul 6, 2018
@fpagnoux fpagnoux requested review from Anna-Livia, MattiSG and sandcha and removed request for Anna-Livia July 6, 2018 16:27
@fpagnoux fpagnoux requested a review from Anna-Livia July 9, 2018 20:56
@@ -1,4 +1,6 @@
# The social_security_contribution is calculated with a marginal scale. Marginal scales are represented with a specific structure.
description: Social security contribution tax scale
unit: currency:EUR
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is ambiguous: the thresholds are in EUR, but the rates are unit-less! See issue raised by france (in FR, sorry)

* Details:
- Implement housing tax minimal amount

<!-- -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have this separation ?

Copy link
Member Author

@fpagnoux fpagnoux Jul 13, 2018

Choose a reason for hiding this comment

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

Because this PR does two independent things from the user point of view : enriching a formula, and adding metadata to parameters.

(These are not so independent from our developper point of view, as the whole point of adding the minimal amount for the housing tax is to have a yaml file with several parameters that we can use for tests)

@@ -52,6 +52,9 @@ def formula(household, period, parameters):
january = period.first_month
accommodation_size = household('accommodation_size', january)

tax_params = parameters(period).taxes.housing_tax
Copy link
Contributor

Choose a reason for hiding this comment

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

proposition :

minimal_amount = parameters(period).taxes.housing_tax.minimal_amount
calculated_amount = accommodation_size * parameters(period).taxes.housing_tax.rate
tax_amount = max_(calculated_amount, minimal_amount)

Copy link
Member Author

Choose a reason for hiding this comment

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

I find the repetition of parameters(period) quite heavy.
I edited the formulas to be a little clearer, let me know if it's enough.

setup.py Outdated
@@ -16,7 +16,7 @@
url='https://github.com/openfisca/openfisca-country-template',
include_package_data = True, # Will read MANIFEST.in
install_requires=[
'OpenFisca-Core >= 23.1, < 24.0',
'OpenFisca-Core >= 23.2, < 24.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Il faut mettre à jour la dépendance à Core

@fpagnoux fpagnoux requested a review from Anna-Livia July 13, 2018 15:17
@fpagnoux fpagnoux merged commit 0ea6e9e into master Jul 16, 2018
@fpagnoux fpagnoux deleted the parse-metadata branch July 16, 2018 19:24
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.

4 participants