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

Supprime les attributs de type base_function #1261

Merged
merged 26 commits into from
Apr 2, 2019
Merged

Supprime les attributs de type base_function #1261

merged 26 commits into from
Apr 2, 2019

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Jan 20, 2019

Connected to openfisca/openfisca-core#460

  • Evolution technique non rétrocompatible
  • Zones impactées : plusieurs (voir liste ci-dessous).
  • Détails :
    • Supprime l'attribut base_function des variables garde_alternee, age, age_en_mois, rsa_isolement_recent, contrat_de_travail_debut, contrat_de_travail_fin, salarie_regime_alsace_moselle, entreprise_creation, prevoyance_obligatoire_cadre_taux_employe, prevoyance_obligatoire_cadre_taux_employeur, livret_a, epargne_revenus_non_imposables, epargne_revenus_imposables, valeur_patrimoine_loue, valeur_immo_non_loue, valeur_terrains_non_loues, valeur_locative_terrains_non_loues
      • ces variables n'avaient jusqu'à présent pas besoin d'être définies sur la période pour laquelle on demandait l'évaluation d'une variable en dépendant: on pouvait remonter dans le passé, voire selon les cas aller dans l'avenir pour y récupérer une valeur définie à un autre moment; ces possibilités rarement utilisées faisaient doublon avec les options possibles pour set_input
      • si vous utilisez ces variables en entrée, vous devrez expliciter la période pour laquelle ces variables sont fournies, de sorte qu'elle recouvre toute la période de vos calculs (vous pouvez utiliser la syntaxe des périodes multi-mois ou multi-années pour gagner en concision)
    • Adapte certains tests qui s'appuyaient sur ces inférences "magiques" pour les variables d'entrée.

Connected to openfisca/openfisca-core#813


Ces changements (effacez les lignes ne correspondant pas à votre cas) :

  • Modifient des éléments non fonctionnels de ce dépôt (tests).

Quelques conseils à prendre en compte :

Et surtout, n'hésitez pas à demander de l'aide ! :)

@fpagnoux
Copy link
Member

Attention, ce changement n'est pas une amélioration technique mais un changement non rétro-compatible exigeant une adaptation de la part des utilisateurs de ces variables.

@Morendil
Copy link
Contributor Author

Description modifiée dans ce sens.

@sandcha
Copy link
Contributor

sandcha commented Jan 29, 2019

Branche rebasée suite à l'évolution de l'intégration continue (suppression du build python 2).

@benjello
Copy link
Member

Je me pose une question: ne risque-t-on pas d'occuper beaucoup plus de mémoire en n'utilisant plus base_function ? Le dispatch_by_period ne va-t-il pas dupliquer beaucoup trop des vecteurs identiques ou n'utilise-t-on que des pointeurs dans ce cas là ?

@benjello
Copy link
Member

Ce genre de changement plaide pour l'existence d'un test de performance mémoire pour voir si on ne dégrade pas le code de ce point de vue là.

@fpagnoux
Copy link
Member

Le risque était bien réel, mais a été anticipé: openfisca/openfisca-core#827 🙂.

@Morendil Morendil force-pushed the remove-rplonv branch 2 times, most recently from d4c2b0b to ca1a00c Compare April 1, 2019 14:04
@sandcha sandcha self-requested a review April 1, 2019 17:03
Copy link
Contributor

@sandcha sandcha left a comment

Choose a reason for hiding this comment

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

Principale demande de changement (ou de confirmation) sur la périodicité de apprentissage_contrat_debut.

@@ -195,7 +195,7 @@ class apprentissage_contrat_debut(Variable):
value_type = date
entity = Individu
label = u"Date de début du contrat d'apprentissage"
definition_period = MONTH
definition_period = ETERNITY
Copy link
Contributor

Choose a reason for hiding this comment

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

Maintenir MONTH ?
D'après le site du Ministère du Travail, un individu peut avoir des contrats d'apprentissage successifs au cours de sa vie.

* Détails :
- Supprime l'attribut base_function des variables `garde_alternee`, `age`, `age_en_mois`, `rsa_isolement_recent`, `contrat_de_travail_debut`, `contrat_de_travail_fin`, `salarie_regime_alsace_moselle`, `entreprise_creation`, `prevoyance_obligatoire_cadre_taux_employe`, `prevoyance_obligatoire_cadre_taux_employeur`, `livret_a`, `epargne_revenus_non_imposables`, `epargne_revenus_imposables`, `valeur_patrimoine_loue`, `valeur_immo_non_loue`, `valeur_terrains_non_loues`, `valeur_locative_terrains_non_loues`
- Adapte certains tests qui s'appuyaient sur ces inférences "magiques" pour les variables d'entrée.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Adapte le comportement de répartition de valeur d'entrée à `set_input_dispatch_by_period` pour les variables `rsa_isolement_recent`, `livret_a`, `epargne_revenus_non_imposables`, `valeur_locative_immo_non_loue`, `valeur_locative_terrains_non_loues`.

@Morendil Morendil requested a review from sandcha April 2, 2019 15:26
@@ -102,6 +102,7 @@ class age_en_mois(Variable):
label = u"Âge (en mois)"
is_period_size_independent = True
definition_period = MONTH
set_input = set_input_dispatch_by_period
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

@sandcha
Copy link
Contributor

sandcha commented Apr 2, 2019

Thank you @Morendil 🎉

@Morendil Morendil merged commit 0406a07 into master Apr 2, 2019
@Morendil Morendil deleted the remove-rplonv branch April 2, 2019 15:46
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