Enforce simplify-periods PR from openfisca-core #685

michelbl · 2017-02-16T18:28:14Z

Connected to openfisca/openfisca-core#442

Add attribute period_behavior
Remove period = period.this_***
Resolve period missmatches
Resolve tricky cases

cbenz · 2017-02-16T18:32:44Z

openfisca_france/model/prestations/prestations_familiales/base_ressource.py

@@ -168,7 +166,9 @@ def nb_enf(famille, period, age_min, age_max):
    Renvoie le nombre d'enfant au sens des allocations familiales dont l'âge est compris entre ag1 et ag2
    """

-    period = period.this_month
+    assert period.unit == u'month'
+    assert period.size == 1


Why did you put those assertions here to replace period = period.this_month and not elsewhere?

Because nb_enf is a helper function that is used in other modules. Do you see a better solution ?

Oops, I did not see that because of the diff display. All right then.

I don't want to open here a debate on helpers vs variables, but we're right in here.

michelbl · 2017-02-17T11:03:51Z

https://github.com/openfisca/openfisca-france/blob/master/openfisca_france/model/prestations/minima_sociaux/aah.py#L79

pensions_alimentaires_percues is a yearly variable that comes from CERFA. Its value is requested for a period of 3 months. I think this is a usage of "add_divide"

benjello · 2017-02-17T13:37:41Z

When pensions_almimentaires_percues comes from fiscal data it is yearly (CERFA). When used for some prestations it is its monthly value that is needed .
When you need the value of a parameter at the month level it is better to use the smaller unit in the definition of the variable.
There might be problem arising from filling the value (initialisation) and or memory management (stock the same value) but it is be context dependent.

michelbl · 2017-02-17T13:43:06Z

Because of the explicit period management that is now required, a month must be given to call a monthly variable from a yearly variable. I use janvier (same as before but explicit now) whenever it is needed :

janvier = period.this_month
age = simulation.calculate('age', janvier)

michelbl · 2017-02-17T13:49:20Z

@benjello on second thought that is exactly what I concluded and what I implemented.

Filling the value is correctly handled by set_input_divide_by_period.

There is redondancy but this can be adressed by numpy references. However I am reluctant to address optimization issues in this PR.

michelbl · 2017-02-24T12:06:43Z

All the tests pass in my development environment, however that is not the case for travis. Somebody has a hint ?

Edit : this is certainly caused by travis installing openfisca-core/master instead of the new version still not merged.

Edit : travis now installs the correct version, but a test fails. I can't understand why :

some tests are ran by travis but not in my dev environment
the first failure stops tests but not in my dev env
I get the name of the yaml test case in my dev env in case of failure, but not in the travis output 😕

Edit : I needed to install inversion_revenus in my dev environment.

fpagnoux · 2017-03-02T13:32:25Z

...e/model/prelevements_obligatoires/prelevements_sociaux/cotisations_sociales/travail_prive.py

@@ -748,9 +757,10 @@ class taille_entreprise(Variable):
    entity = Individu
    label = u"Catégorie de taille d'entreprise"
    url = u"http://www.insee.fr/fr/themes/document.asp?ref_id=ip1321"
+    period_unit = MONTH
+    calculate_output = calculate_output_first_month


Si le seul but est de faire passer ce test, autant modifier le test, vu que la période y est arbitraire.

fpagnoux · 2017-03-02T13:50:22Z

openfisca_france/model/prelevements_obligatoires/impot_revenu/ir.py



 class age_en_mois(Variable):
    base_function = missing_value
    column = AgeCol(val_type = "months")
    entity = Individu
    label = u"Âge (en mois)"
+    period_unit = MONTH
+    calculate_output = calculate_output_first_month


Y a-t-il vraiment un sens à calculer un age_en_mois pour une année ? Ce test le fait, mais le test sur les valeurs annuelles me paraît peu pertinent.

fpagnoux · 2017-03-02T14:02:46Z

openfisca_france/model/prelevements_obligatoires/impot_revenu/ir.py



 class enfant_a_charge(Variable):
    column = BoolCol
    entity = Individu
    label = u"Enfant à charge non marié, de moins de 18 ans au 1er janvier de l'année de perception des" \
        u" revenus, ou né durant la même année, ou handicapés quel que soit son âge"
+    period_unit = MONTH
+    calculate_output = calculate_output_first_month


Cette variable semble très liée à l'impôt , et donc plutôt annuelle. Elle est appelée une seule fois par une variable mensuelle (dans cmu.py). Plutôt que de la définir en mensuelle et de rajouter un calculate_output, je suggère plutôt que l'unique appellant mensuel s'adapte et qu'on la définisse en annuel.

fpagnoux

Besoin d'une discussion IRL pour voir si ces 4 tests sont vraiment à déprécier 😏 :

test_mes_aides_5591635b7ae4c40130761eb7
test_mes_aides_54c2259755fa25a535d69571
test_mes_aides_54be84abdee70bd172bf3ba3
test_mes_aides_5421322ffb35830200b3133b

fpagnoux · 2017-03-03T17:51:37Z

requirements.txt

@@ -1,5 +1,5 @@
 # Add Biryani extra requirement here because it is needed by OpenFisca-Core and installation from Git URL below
 # does not take extras_require of Core setup.py into account.
 Biryani[datetimeconv] >= 0.10.4
--editable git+https://github.com/openfisca/openfisca-core.git#egg=OpenFisca-Core
+--editable git+https://github.com/openfisca/openfisca-core.git@simplify-periods#egg=OpenFisca-Core


Si c'est temporaire, attention à bien penser à le reverter.

Oui bien vu ! Il faut surtout pas merger ça.

Quand je commit ce genre de chose, j'ai pour habitude de préfixer le commit par un gros [TO REVERT], pour que ce soit plus facilement visible à la review.

Remove period = period.this_* when this is useless.

Use a period that matches the attribute period_behavior of the called variable, or use an option (ADD, DIVIDE).

cbenz reviewed Feb 16, 2017

View reviewed changes

michelbl force-pushed the simplify-periods branch from 306d7a8 to a97f730 Compare February 20, 2017 17:46

michelbl requested a review from fpagnoux February 24, 2017 14:00

michelbl self-assigned this Feb 24, 2017

fpagnoux self-assigned this Feb 24, 2017

michelbl changed the title ~~WIP: Enforce simplify-periods PR from openfisca-core~~ Enforce simplify-periods PR from openfisca-core Feb 27, 2017

MattiSG changed the title ~~Enforce simplify-periods PR from openfisca-core~~ Enforce simplify-periods PR from openfisca-core Feb 28, 2017

michelbl force-pushed the simplify-periods branch 2 times, most recently from e7872bd to 0cfc353 Compare March 2, 2017 17:24

fpagnoux reviewed Mar 2, 2017

View reviewed changes

fpagnoux requested changes Mar 3, 2017

View reviewed changes

fpagnoux force-pushed the simplify-periods branch from 295e63f to 2c23a59 Compare March 3, 2017 16:22

fpagnoux requested changes Mar 3, 2017

View reviewed changes

michelbl force-pushed the simplify-periods branch 2 times, most recently from 9510a58 to fcece09 Compare March 6, 2017 14:10

Michel Blancard added 10 commits March 6, 2017 18:13

Add attribute period_behavior

c8bbbd9

Remove period = period.this_* when this is useless.

Use explicit periods in variable calls

762b7a5

Use a period that matches the attribute period_behavior of the called variable, or use an option (ADD, DIVIDE).

Compute separately monthly and yearly variables

7edc8a8

Fix rsa_base_ressources_patrimoine_individu

f9cc8fb

Set set_input hooks

73332be

Remove calculate_add_divide in test

cf312f2

Adapt test_entities.py to period_behavior

d79d318

Adapt test_legacy_entities.py to period_behavior

0f2c3c8

Adapt test_nb_enfants.py to period_behavior

9c30d01

Change set_input hook for variable age

ec6d6d3

Michel Blancard and others added 20 commits March 6, 2017 18:23

Import period units from periods.py

a9d6eb7

Rename period_behavior to period_unit

b26d940

Rename PERMANENT to ETERNITY

8476b4b

Fix imports in reform

9418a97

Remove attribute is_permanent

164255f

Define a local period order

20f6930

Rename period_unit to definition_period

b061a3e

Remove hook for age_en_mois

0a21432

Make enfant_a_charge a year defined variable

e7fb397

Remove hook for taille_entreprise

85c0b81

Adapt mes-aides test to new period constraints

8395f72

Remove hook for age

e6e76d0

Do not import calculate_output_first_month

25046a3

Make exoneration_is_creation_zrr a yearly variable

476a3ff

Rename period.this_month to period.first_month

5087a78

Remove redundant quarters inputs in tests

c2358d9

Remove redundant year inputs in tests

ec5027b

Fix test test_mes_aides_5591635b7ae4c40130761eb7

a767a14

Make AL more robust to division rounding errors

096d843

Don't count twice revenus locatifs in RSA

72761b4

fpagnoux force-pushed the simplify-periods branch from fcece09 to 72761b4 Compare March 6, 2017 17:25

fpagnoux approved these changes Mar 6, 2017

View reviewed changes

fpagnoux added 2 commits March 6, 2017 18:49

Fixup missing period in #695

9e13be9

Add link to PR in Changelog.md

21008ee

fpagnoux force-pushed the simplify-periods branch from 5496ea4 to 21008ee Compare March 6, 2017 17:50

fpagnoux mentioned this pull request Mar 6, 2017

Simplify boilerplate #697

Merged

fpagnoux merged commit 97a8ba6 into master Mar 6, 2017

fpagnoux deleted the simplify-periods branch March 6, 2017 18:08

This was referenced Apr 11, 2017

Fournir la valeur de tehr de manière annuelle et non mensuelle #411

Closed

Problèmes avec l'ARS #456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce simplify-periods PR from openfisca-core #685

Enforce simplify-periods PR from openfisca-core #685

michelbl commented Feb 16, 2017 •

edited by fpagnoux

Loading

cbenz Feb 16, 2017

michelbl Feb 17, 2017

cbenz Feb 17, 2017

michelbl commented Feb 17, 2017 •

edited

Loading

benjello commented Feb 17, 2017

michelbl commented Feb 17, 2017

michelbl commented Feb 17, 2017 •

edited

Loading

michelbl commented Feb 24, 2017 •

edited

Loading

fpagnoux Mar 2, 2017

fpagnoux Mar 2, 2017

fpagnoux Mar 2, 2017

fpagnoux left a comment •

edited

Loading

fpagnoux Mar 3, 2017

michelbl Mar 3, 2017

fpagnoux Mar 6, 2017

fpagnoux Mar 6, 2017

Enforce simplify-periods PR from openfisca-core #685

Enforce simplify-periods PR from openfisca-core #685

Conversation

michelbl commented Feb 16, 2017 • edited by fpagnoux Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelbl commented Feb 17, 2017 • edited Loading

benjello commented Feb 17, 2017

michelbl commented Feb 17, 2017

michelbl commented Feb 17, 2017 • edited Loading

michelbl commented Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelbl commented Feb 16, 2017 •

edited by fpagnoux

Loading

michelbl commented Feb 17, 2017 •

edited

Loading

michelbl commented Feb 17, 2017 •

edited

Loading

michelbl commented Feb 24, 2017 •

edited

Loading

fpagnoux left a comment •

edited

Loading