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

Factorise la duplication dans le calcul de la 'Prime de Noël' #1149

Merged
merged 10 commits into from
Oct 15, 2018

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Oct 8, 2018

  • Amélioration technique.
  • Périodes concernées : à partir du 01/01/2002.
  • Zones impactées : openfisca_france/model/prestations/minima_sociaux/aefa.py.
  • Détails :
  • Factorise le code dupliqué (tripliqué d'ailleurs) du calcul de l'aide exceptionnelle de fin d'année.
  • Corrige le montant de base de l'AEFA
  • Prolonge l'AEFA au-delà de 2015 car elle a été reconduite
  • N'applique la majoration familiale que pour les bénéficiaires du RSA

Fixes #569


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

  • Corrigent ou améliorent un calcul déjà existant.

Quelques conseils à prendre en compte :

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

Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

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

Il me semble que null indique que le paramètre n'existe plus ce qui est le cas. La prime forfaitaire n'existe qu'en 2008.

Ne doit-on pas garder ce null et mettre une condition explicite ? Je pose juste la question hein.
Et si je peux me permettre des améliorations opportunistes:

  • remplacer P par aefa
  • aefa par montant_aefa
  • Virer les dummy et rassembler la condition et comparasion

Et merci beaucoup !

2008-01-01:
value: 67.55
2009-01-01:
value: null
value: 0
Copy link
Member

Choose a reason for hiding this comment

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

👍 sur ce que dit @benjello, si le paramètre n'existe plus à partir de 2009, il faut garde le null 🙂

@Morendil
Copy link
Contributor Author

J'ai nettoyé encore mais pour le "null" je ne vois pas comment m'y prendre, je ne sais pas comment tester l'existence de la valeur d'un paramètre législatif.

@Morendil
Copy link
Contributor Author

En fait ce paramètre est mal nommé. Maintenant que la formule est unifiée pour toutes les périodes, sa signification est "montant de la prime exceptionnelle". Il devrait s'appeler "prime_exceptionnelle". Il se trouve simplement qu'il n'a été activé qu'une seule fois, en 2008. Je modifie dans ce sens.

@benjello
Copy link
Member

Oui @Morendil c'est exactement cela !

@benjello
Copy link
Member

@Morendil : il faudrait faire débuter la valeur la seule année où elle est non-nulle et l'éteindre juste après.
@fpagnoux @sandcha comment tester proprement l'existence d'un paramètre ?
On peut aussi mettre un teste sur period pour ne ne rajouter la prime exceptionnelle que pour l'année exceptionnelle.

@Morendil
Copy link
Contributor Author

J'ai corrigé quelques problèmes supplémentaires comme indiqués dans les commits et répercutés dans le descriptif de la PR. J'hésite désormais entre une mise à jour PATCH ou MINOR, je suis preneur de vos avis.

@benjello En fait pour "comment tester la présence d'un paramètre" j'avais la réponse sous les yeux, c'est hasattr() quelques lignes plus haut. Cela dit je pense que ce n'est tout simplement pas souhaitable dans le cas présent, d'une part parce que l'AEFA continue à être versée, et qu'il n'est pas exclu qu'un "coup de pouce" lui soit donné une nouvelle fois; d'autre part pour des raisons plus directement liées à la qualité du code: je préfère un code sans if dont le comportement est régulé par la donnée.

Je ne comprends pas ton insistence pour utiliser des valeurs null, que je préfèrerais fortement éviter (c'est pour certains l'erreur qui valait un milliard de dollars). Est-ce qu'il s'agit d'une convention sur la rédaction des formules du modèle, sur laquelle il existe une jurisprudence forte et/ou que nous aurions documentée ? Si oui je n'étais pas au courant et je veux bien des pointeurs, même si mon avis est opposé. Est-ce parce que tu anticipes des difficultés qui pourraient survenir lors de l'import des barèmes IPP, que je connais mal ? Si c'est le cas je veux bien qu'on en discute dans le contexte de cette importation. Bref je suis ouvert à mieux comprendre tes objections, soit pour changer d'avis moi-même, soit pour t'amener à changer d'avis sur la base de solutions alternatives à celles-ci.

Cela dit il me semble que cette PR améliore déjà considérablement le code et qu'on pourrait la merger en attendant d'avoir cette discussion. (Si c'est moi qui change d'avis, je m'engage à refaire une passe sur ce code pour le mettre au standard convenu dans l'équipe.)

@benjello
Copy link
Member

@Morendil : s'il n'y a pas de null c'est que la dernière valeur prévaut. S'il y a null c'est que le paramètre n'existe plus et c'est de l'information que l'on veut garder. Et oui c'est de l'information explicite des barèmes IPP où il y aura beaucoup de valeurs qui ne seront pas égale à 0 avant de devenir ̀null`.

Par ailleurs concernant le fond, j'avais en tête que la prime de Noël avait été arrêtée il y a quelques années ce que tu sembles contredire. Hors personne du côté de mes aides n'a réagit lorsqu'elle fût supprimée (d'openfisca). Je me permets de signaler cela à @guillett. Mais je valide ta modification acr une rapide recherche me montre qu'elle a bien été versée en 2016 et 2017 ...

Après pas d'objection pour merger tel quel si l'on recense les modifications qu'il reste à régler dans une autre issue (si c'est al bonne façon de faire).

@Morendil
Copy link
Contributor Author

@benjello Deal. :)

Pour la suppression ça remonte à #770 qui était une évolution de syntaxe touchant le modèle de façon transverse et je n'ai pas l'impression à la lecture des discussion que l'équipe Mes Aides ait participé à ces décisions. Cela pose la question de la préservation de la fonctionnalité du modèle lors d'évolutions "techniques", peut-être en étoffant la checklist de review, peut-être aussi en trouvant le moyen de découper même ces gros morceaux en PR plus petites.

@Morendil Morendil merged commit 0a19e6d into master Oct 15, 2018
@Morendil Morendil deleted the refactor-aefa branch October 15, 2018 21:51
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