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

Correction du calcul de la PAJE #1110

Merged
merged 11 commits into from
Sep 21, 2018
Merged

Correction du calcul de la PAJE #1110

merged 11 commits into from
Sep 21, 2018

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Sep 11, 2018

  • Correction d'un bug
  • Périodes concernées : à partir du 01/01/2014.
  • Zones impactées : prestations/prestations_familiales/paje.py.
  • Détails :
    • Corrige le calcul de la PAJE lorsqu'on l'évalue pour un vecteur de familles.
    • Simplifie et uniformise la formule de calcul pour toutes les périodes.
    • Enrichit la base de tests de la PAJE pour augmenter la couverture

Fixes #1109

@Morendil Morendil requested review from frtomas, guillett, fpagnoux and a team September 12, 2018 08:03
@bfabre01
Copy link

@Morendil : merci pour ces modifs. Pour répondre à ta question sur #1109 , l'allocation de base de la paje ne peut être attribuée qu'à un seul enfant de moins de 3 ans à la fois (excepté pour les naissances multiples et adoptions simultanées, ce qui n'est d'ailleurs pas pris en compte). La condition sur est_plus_jeune_enfant impliquait que s'il y a plus d'un enfant de moins de 3 ans dans la famille, dont au moins un est né avant le 01/04/2014 et un autre après, on attribue la paje au titre de celui né après le 01/04/2014. Avec tes modifs, on fait l'inverse. Je ne sais pas quelle est la meilleure solution. En revanche, je pense qu'il faut marquer cette hypothèse dans la docstring.

@bfabre01
Copy link

Autre point que j'ai remarqué au passage, il y a maintenant des barèmes de l'allocation de base de la PAJE pour les enfants nés avant le 1er avril 2018 et d'autres pour ceux nés après. Mais je n'ai pas l'impression que ces changements aient été codés dans OpenFisca.

@benjello
Copy link
Member

En réponse à @bfabre01 : la nouvelle PAJE est dans les tuyaux #994.
@openfisca/france-msa : pourriez-vous nous renseigner sur la règle d'attribution selon la date de naissance soulevée par @bfabre01

cc @guillett

@bfabre01
Copy link

@Morendil : quand j'applique tes modifs aux données, on passe à 3,7 milliards de dépenses. Donc, ça résout le bug.

@Morendil
Copy link
Contributor Author

@bfabre01 Merci pour les précisions sur le critère "le plus jeune enfant", c'est plus clair comme ça. Ma "correction" n'est est donc pas une ou en tout cas pas complètement. Je vais regarder également #994.

@benjello
Copy link
Member

benjello commented Sep 13, 2018

@Morendil : #994 a été mergé. Pourrais tu rebaser, corriger le problème avec le min et resoumetter à review pour merge rapide ? On aimerait transférer tout cela sur le centre d'accès sécurisé à distance (CASD) ASAP.

Merci d'avance !

@Morendil
Copy link
Contributor Author

Morendil commented Sep 14, 2018

La nouvelle version que je viens de pousser est rebasée (donc inclut #994), supprime environ 200 lignes de code, ramène le calcul de la PAJE avec les 2 réformes (2014 et 2018) à une seule formule unifiée avec ce qui me semble être la bonne façon de vectoriser l'arbre de décision qui implique à la fois la période et la date de naissance de l'enfant pour lequel la famille est bénéficiaire. (Cette formule ne traite toujours pas certains cas métier: enfants adoptés, naissances ou adoptions multiples.)

Les cas de test existants restent passants, mais je reste très sceptique quant à l'adéquation de la couverture de tests par rapport à la complexité du sujet. J'ai bien conscience que couvrir toute la combinatoire métier (plafonds différents selon le nombre d'enfants, l'activité des parents, la date de naissance, la période, etc.) serait extrêmement fastidieux en l'état actuel de ce que permet le test runner. Par conséquent il me semble qu'il serait judicieux d'investir dans de nouvelles possibilités pour le test runner.

@Morendil Morendil changed the title Correction du calcul de la PAJE (WIP) Correction du calcul de la PAJE Sep 14, 2018
@bonjourmauko
Copy link
Member

bonjourmauko commented Sep 14, 2018

Cette formule ne traite toujours pas certains cas métier: enfants adoptés, naissances ou adoptions multiples

Et ce n'est pas à mon avis une regression : en effet, par exemple, en essayant de prendre en compte la cumulation de base prévue par la loi en cas de grossesse multiple, en se rend vite compte qu'elle n'a pas été pris en compte.

Essaie naïf :

        age_plus_jeune_individu = famille.min(famille.members('age', period))
        est_plus_jeune_individu = famille.members('age', period) == age_plus_jeune_individu

        individu_elig_avant_avril_2014 = enfant_eligible_ne_avant_avril_2014() * est_plus_jeune_individu
        montant_elig_avant_avril_2014 = montant_enfant_ne_avant_avril_2014()
        individu_elig_apres_avril_2014 = enfant_eligible_ne_apres_avril_2014() * est_plus_jeune_individu
        montant_elig_apres_avril_2014 = montant_enfant_ne_apres_avril_2014()

        montant = (
            individu_elig_avant_avril_2014 * montant_elig_avant_avril_2014 +
            not_(individu_elig_avant_avril_2014) * individu_elig_apres_avril_2014 * montant_elig_apres_avril_2014
            )

        return array(montant.sum())

qui prend un vecteur d'individus, mais fais un comparaison par âge quand cela devrait être par date de naissance, tombe sans surprise sur un erreur de broadcasting pour un vecteur des familles (shape n > 1,).

@@ -211,3 +245,66 @@
# BMAF 2013 = 403.79
# BMAF 2016 = 406.62
paje_base: 0.4595 * 403.79

- name: "Calcul de la PAJE sur un vecteur de > 1 famille"
Copy link
Member

Choose a reason for hiding this comment

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

@Morendil @benjello Il me semble que ce test n'a d'autre function que de vérifier que le calcul vectorielle marche, n'ajoutant pas de « cas métier ». Ne serait-il plus lisible d'ajouter une famille à tous le cas métier déjà couverts ?

Copy link
Member

Choose a reason for hiding this comment

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

Oui c'est une solution simple qui permettrait de détecter certaines erreurs criantes de vectorisation.
L'autre solution serait d'empiler tous les cas métiers pour faire une seule simulation vectorisée (pas facile) !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ca dépend comment tu entends "ajouter une famille". Si c'est automatiquement, par le biais d'une modification du test runner pour vérifier que les calculs singuliers et vectoriels sont iso, alors oui… mais si la proposition c'est d'ajouter une famille (différente) à chaque cas métier, alors non : l'écriture de tests YAML est déjà assez fastidieuse comme ça, si on en rajoute une couche on va encore plus décourager les tests dans les contributions.

Je vois du "leverage" dans une équipe Core de très bon niveau qui investit temps et énergie à ajouter des features à Core, et notamment au test runner, qui facilitent la contribution de formules de qualité bien testées par des contributeurs plus périphériques, et par des country teams qui se concentrent sur l'expressivité des formules de leurs pays respectifs.

Copy link
Member

Choose a reason for hiding this comment

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

J'ai émis mon avis en pensant que Mauko pensait à ta première solution.
Peut-être suffirait-il de dédoubler systématiquement le cas type testé pour tester la vectorisation ?
Cela doit être facile à faire et le temps de calcul des tests ne sera pas vraiment plus long.

Copy link
Member

Choose a reason for hiding this comment

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

Oui je pensais au test runner, en prenant comme MVP de le faire à la main pour voir ce que cela donnait.

@Morendil
Copy link
Contributor Author

Merci @maukoquiroga - pas de régression avec la couverture de tests supplémentaire, ça me semble fort bon signe. GTM pour moi @benjello et toujours curieux des résultats lors du test au CASD.

@benjello
Copy link
Member

benjello commented Sep 18, 2018

ok on regardera cela avec @bfabre01 et on revient vers toi.
Mais on peut merger entre-temps car on va pas faire cela très rapidement.

@Morendil
Copy link
Contributor Author

@benjello Je souhaite partager de la confusion et de la perplexité. ;)

J'ai du mal à interpréter ton commentaire qui dit "ok on regardera cela avec @bfabre01 et on revient vers toi. Mais on peut merger entre-temps car on va pas faire cela très rapidement." et encore plus de mal quand ça s'ajoute au constat que cette PR n'a toujours pas de review. En bref: on merge ou on merge pas !?

Je partage aussi de la douleur à constater que mes modifs à setup.py et Changelog sont obsolètes et que je vais avoir à les refaire. J'aimerais que le collectif Openfisca prenne conscience que c'est un tue-l'amour pour la contribution.

J'ai une proposition concrète: désigner pour OF-Fr un rôle de "pull request herder". La personne qui occupe ce rôle décide de l'ordre dans lequel on merge les PR. C'est elle qui incrémente les compteurs de version et remplit le Changelog à partir du commentaire en en-tête de PR. (Les personnes qui ont fait les PR restent responsable de la rédaction de cet en-tête.)

@benjello
Copy link
Member

@Morendil : on est charette ailleurs (PLF, ca ira mieux dans 1( jours) donc merge !
Mais on est conscient du travail et qu'il faille qu'on le regarde pour être sûr de tout bien comprendre.
Bref on apprécie tes efforts et on en veut pas les entraver.

@Morendil
Copy link
Contributor Author

Je veux bien merger mais j'ai besoin d'un approve…

@benjello
Copy link
Member

Et ok pour la proposition d'améliorer la gouvernance des PRs ! Débattons-en !

@Morendil Morendil merged commit d4a7c2c into master Sep 21, 2018
@bonjourmauko bonjourmauko deleted the fix-paje branch September 21, 2018 14:41
@bonjourmauko
Copy link
Member

Merci @Morendil et bravo pour le travail fantastique que tu as fait sur la PAJE, much kudos.

Je partage aussi de la douleur à constater que mes modifs à setup.py et Changelog sont obsolètes et que je vais avoir à les refaire.

C'est un cas recurrent. Cette communauté à l'habitude de laisser l'OP d'une PR faire le merge après qu'un pull request a été acceptée. De mon expérience, quand c'est le reviewer qui prends la charge de le faire c'est beaucoup plus naturel, or cela peut ralentir encore davantage les reviews.

J'aimerais que le collectif Openfisca prenne conscience que c'est un tue-l'amour pour la contribution.

👍

J'ai une proposition concrète: désigner pour OF-Fr un rôle de "pull request herder". La personne qui occupe ce rôle décide de l'ordre dans lequel on merge les PR. C'est elle qui incrémente les compteurs de version et remplit le Changelog à partir du commentaire en en-tête de PR. (Les personnes qui ont fait les PR restent responsable de la rédaction de cet en-tête.)

Discutons-en, c'est en effet très intéressant car c'est une sous-specialisation du maintainer, et il y a effectivement des questions qui vont se poser, notamment une pour moi : si c'est un travail laborieux, notamment lors de rebases error-prone, cela serait intéressant de faire tourner cette casquette.

STP, n'hésites pas à ouvrir une issue pour que l'on puisse continuer cette discussion à toutes fins utile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants