Les Enums sont définis comme en Python 3 #846
Conversation
|
Merci beaucoup @Anna-Livia. J'ajoute deux cas de test supplémentaires. Dans la situation suivante la variable {
"familles": {
"_": {
"parents": [
"demandeur"
],
"rsa_non_calculable": {
"2017-10": null
},
"aide_logement_non_calculable": {
"2017-12": null
}
}
},
"foyers_fiscaux": {
"_": {
"declarants": [
"demandeur"
],
"personnes_a_charge": []
}
},
"individus": {
"demandeur": {}
},
"menages": {
"_": {
"personne_de_reference": [
"demandeur"
]
}
}
}Pour la situation suivante je m'attends à un RSA à 470.95€ (https://mes-aides.gouv.fr/tests/58d3eeeeb5028ff3207a188e/show) mais j'obtiens 0 (désolé pour le dump complet [avec les variables d'extensions]) : |
CHANGELOG.md
Outdated
| @@ -1,5 +1,28 @@ | |||
| # Changelog | |||
|
|
|||
| ## 19.0.0 - [#846](https://github.com/openfisca/openfisca-france/pull/846) | |||
There was a problem hiding this comment.
Le changelog n'explique pas quels sont les breaking change et comment s'y adapter
CHANGELOG.md
Outdated
|
|
||
| * Amélioration technique | ||
| * Détails : | ||
| - Modifie la façon dont les Enumerations sont définis et appelés. |
There was a problem hiding this comment.
J'aurais tendance à dire que "Enumerations" est féminin. définies ? appelées ?
| @@ -107,12 +102,14 @@ def formula(individu, period, parameters): | |||
| # heures_mensuelles = min_(salaire_mensuel_reference / smic_h_b, 35 * 52 / 12) # TODO: depuis 2001 mais avant ? | |||
| heures_mensuelles = 35 * 52 / 12 | |||
| cho_seuil_exo = law.prelevements_sociaux.contributions.csg.chomage.min_exo * heures_mensuelles * smic_h_b | |||
|
|
|||
| eligible = \ | |||
There was a problem hiding this comment.
Prefer implicit line continuation to backslashes.
eligible = (
(taux_csg_remplacement == TypesTauxCSGRemplacement.taux_reduit) +
(taux_csg_remplacement == TypesTauxCSGRemplacement.taux_plein)
)| @@ -22,7 +23,14 @@ def formula(self, simulation, period): | |||
| seuil_effectif = simulation.parameters_at(period.start).cotsoc.versement_transport.seuil_effectif | |||
|
|
|||
| preload_taux_versement_transport() | |||
| public = (categorie_salarie >= 2) | |||
| public = \ | |||
| @@ -425,14 +429,25 @@ def switch_on_allegement_mode(simulation, period, mode_recouvrement, variable_na | |||
| should precisely be the variable name prefixed with 'compute_' | |||
| """ | |||
| compute_function = globals()['compute_' + variable_name] | |||
| return switch( | |||
| if type(mode_recouvrement[0]) == TypesAllegementCotisationAllocationsFamilialesModeRecouvrement: | |||
There was a problem hiding this comment.
That is quite complicated.
Why not giving up the switch and have something like:
recouvrement_fin_annee = (mode_recouvrement == TypesAllegementCotisationAllocationsFamilialesModeRecouvrement.fin_d_annee) + (mode_recouvrement == TypesAllegementFillonModeRecouvrement.fin_d_annee)
recouvrement_anticipe = (mode_recouvrement == TypesAllegementCotisationAllocationsFamilialesModeRecouvrement. anticipe) + (mode_recouvrement == TypesAllegementFillonModeRecouvrement. anticipe)
recouvrement_progressif = ...
return recouvrement_fin_annee * compute_allegement_annuel(...) + recouvrement_anticipe * compute_allegement_anticipe(...)| plancher = min_zone_1 * (zone_apl == 1) + min_zone_2 * (zone_apl == 2) + min_zone_3 * (zone_apl == 3) | ||
| taux = P.taux.zone1 * (zone_apl == TypesZoneApl.zone_1) + P.taux.zone2 * (zone_apl == TypesZoneApl.zone_2) + P.taux.zone3 * (zone_apl == TypesZoneApl.zone_3) | ||
| plancher = min_zone_1 * (zone_apl == TypesZoneApl.zone_1) + min_zone_2 * (zone_apl == TypesZoneApl.zone_2) + min_zone_3 * (zone_apl == TypesZoneApl.zone_3) | ||
| public = \ |
There was a problem hiding this comment.
Use implicit line continuation
| @@ -725,7 +704,15 @@ def formula(self, simulation, period): | |||
| _P = simulation.parameters_at(period.start) | |||
|
|
|||
| traitement_annuel_brut = _P.fonc.IM_100 | |||
| return (traitement_indiciaire_brut * 100 * 12 / traitement_annuel_brut) * (categorie_salarie >= 2) | |||
| public = \ | |||
There was a problem hiding this comment.
Use implicit line continuation
| @@ -826,22 +813,28 @@ def formula(individu, period, parameters): | |||
| plafond = (plafond_mensuel_1 * (fonc_nbenf == 1) + plafond_mensuel_2 * (fonc_nbenf == 2) + | |||
| plafond_mensuel_3 * (fonc_nbenf == 3) + | |||
| plafond_mensuel_supp * max_(0, fonc_nbenf - 3)) | |||
|
|
|||
| sft = (categorie_salarie >= 2) * (categorie_salarie < 7) * min_( | |||
| public = \ | |||
There was a problem hiding this comment.
Use implicit line continuation
| @@ -861,8 +854,15 @@ def formula(self, simulation, period): | |||
| traitement_indiciaire_brut = simulation.calculate('traitement_indiciaire_brut', period) | |||
| nouvelle_bonification_indiciaire = simulation.calculate('nouvelle_bonification_indiciaire', period) | |||
| categorie_salarie = simulation.calculate('categorie_salarie', period) | |||
|
|
|||
| public = \ | |||
There was a problem hiding this comment.
Use implicit line continuation
| plafond_personne_seule = take(plafonds_by_zone[0], zone_apl) | ||
| plafond_couple = take(plafonds_by_zone[1], zone_apl) | ||
| plafond_famille = take(plafonds_by_zone[2], zone_apl) + (al_nb_pac > 1) * (al_nb_pac - 1) * take(plafonds_by_zone[3], zone_apl) | ||
| index_zone_apl = select( |
There was a problem hiding this comment.
All this preprocessing is getting heavy, and we now have tools to simplify it (indexing parameters with a variable), provided we take advantage of the breaking change to do some smart renaming in the parameters. I'm opening a PR on this branch and I'll let you decide whether we integrate it in this PR or not 🙂
There was a problem hiding this comment.
Voir #856 (et openfisca/openfisca-core#597)
|
@Anna-Livia j'ai fait évoluer les extensions sur mes-aides.gouv.fr et j'ai réussi à faire passer tous les tests. |
CHANGELOG.md
Outdated
| } | ||
| ``` | ||
|
|
||
| #### YAML testing |
There was a problem hiding this comment.
Je trouve cet exemple assez difficile à lire, car il contient beaucoup d'entrées/sorties qui ne sont pas directement liés au changement sur les enums.
Peut-être peut-on le simplifier ?
setup.py
Outdated
| @@ -7,7 +7,7 @@ | |||
|
|
|||
| setup( | |||
| name = 'OpenFisca-France', | |||
| version = '19.0.0', | |||
| version = '19.0.0rc0, | |||
There was a problem hiding this comment.
@Anna-Livia je crois qu'il manque un '.
There was a problem hiding this comment.
There is no point in Python style versionning 🙂
There was a problem hiding this comment.
CHANGELOG.md
Outdated
| input_variables: | ||
| salaire_de_base: 1467 | ||
| # nécessaire pour des requêtes sur un mois de salaire : | ||
| allegement_fillon_mode_recouvrement: anticipe |
There was a problem hiding this comment.
Cette ligne là auss i est nouvelle non ?
openfisca_france/model/base.py
Outdated
| 'non_pertinent', | ||
| ]) | ||
|
|
||
| class TypesStatutOccupationLogement(Enum): |
There was a problem hiding this comment.
Ces tYpes gagneraient à être classés selon l'ordre alphabétique.
CHANGELOG.md
Outdated
| allegement_fillon_mode_recouvrement: anticipe | ||
| effectif_entreprise: 250 | ||
| categorie_salarie: 0 | ||
| contrat_de_travail_duree: cdi |
|
@benjello Les modifications que tu as demandé sont implémentées 🙂 La PR sur les Enums transforme les Enums OpenFisca en Enums Python 3. Les index ne sont plus des inputs compatibles, ils sont remplacés par des strings : On comprends que cela nécessite un important effort d’adaptation de ton côté, si tu le souhaites, nous serions heureux de filer un coup de main dans la migration. Quel est ton estimation de l’effort nécessaire pour la migration de ton coté ? Ce changement devrait beaucoup aider nos nouveaux clients et les nouveaux mainteneurs à aborder et contribuer à OpenFisca. |
There was a problem hiding this comment.
Mon but n'est pas de bloquer pour bloquer comme je l'ai dit à @maukoquiroga mais en plus des changements que cela va engendrer côté chargement des inputs, je suis inquiet de la performance avec des vecteurs numpy de strings tant du point de vue vitesse que mémoire. Je pense que cela doit être un peu étudié avant de basculer sur un changement irréversible.
|
@benjello merci beaucoup pour ton feedback, en regardant les temps passés sur CircleCi (la dernière branche IPP et celle des Enum) on ne voit pas d'impact majeur. Je pense qu'il serait intéressant de lancer les tests mes-aides.gouv.fr d'un seul coup, i.e. à la place de faire 500 tests d'1 situation on va lancer 1 test de 500 situations pour se rapprocher de tes cas d'utilisation. On aura ainsi des informations sur la performance et voir si il y a des changements significatifs. |
|
@fpagnoux : maintenant que tu as une vision large ton avis nous intéresse (je parle au moins pour moi) |
|
Je regarde l'impact mémoire et vitesse d'exécution. @guillett si tu veux avoir des cas d'usages grosse population, tu peux regarder ici : openfisca/openfisca-core#604 |
|
Bilan des tests: Peu d'impact mémoire : on ne stocke pas des vecteurs de strings, mais de référence vers des items d'énums. Ça équivaut à stocker des Par contre, au niveau temps de calcul, ça s'envole pour de grosses populations 😞. Je tente de faire une implem alternative qui garderait la même syntaxe et la même interface (quand c'est possible) mais qui resterait basé sur des entiers sous le capot. |
|
Good job @fpagnoux |
|
Merci @fpagnoux, sad pour la perte de perf effectivement 😢 . Qu'est-ce que tu as en tête pour implémentation alternative ? |
|
Pour garder des performances correctes, il faut que les comparaison de type À partir de ça, deux possibilités qui ont l'air de tenir la route en terme de perf:
Pour le moment, après reflexion, j'ai plutôt envie de partir sur la solution 1 qui me semble la plus simple. |
|
Je pense aussi que la solution 1 est très raisonnable. |
|
Dernier test de perf, pour un calcul du revenu disponible:
Pas de dégradation significative en terme de performances. |
|
En terme d'usage, la façon de déclarer les énums a changé. Il y a une petite dizaine de variables à adapter côté IPP (rapide). @benjello est-ce que c'est bon à merger pour toi ? Je suggère qu'on merge ça avant d'adapter complètement le code côté IPP, afin d'éviter de faire traîner encore plus cette PR qui bloque les dev sur france 🙂 |
Connected to #831
Amélioration technique
Détails :
Par exemple pour :
FalsedevientTypesAAHNonCalculable.non_renseigneTruedevientTypesAAHNonCalculable.intervention_CDAPH_necessaireLes valeurs possibles des Enums ainsi que les nouvelles valeurs par défaut sont disponibles sur legislation.openfisca.fr
Pour les mainteneurs de formules:
Les Enums étaient habituellement placés au dessus de la variable qui le calculait.
Ils sont maintenant tous placés dans le fichier
model/base.py, et commencent tous parTypeCes changements _(effacez les lignes ne correspondant pas à votre cas) impactent l'API publique d'OpenFisca France (par exemple renommage ou suppression de variables).