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

Corrige les modalités de calcul de l'âge #1346

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Jun 14, 2019

  • Amélioration technique.
  • Périodes concernées : toutes.
  • Zones impactées : toutes.
  • Détails :
    • Modifie le calcul de l'âge, en introduisant une méthode set_input qui intercepte les entrées age et age_en_mois pour les transformer en une date de naissance.

Précédemment, ces deux variables effectuaient un calcul compliqué et qui s'appuyait sur des mécanismes internes de Core (via population.get_holder, voir aussi openfisca/openfisca-core#887 qui motive en partie de ne plus accéder directement aux Holder dans France).

Il était possible de fournir age_en_mois et d'obtenir age, mais aussi de fournir age et d'obtenir age_en_mois; ce qui signifie qu'il existait entre ces variables une relation de dépendance circulaire.

Cette PR corrige ces incohérences en passant par un autre mécanisme, à savoir une méthode set_input pour ces deux variables. Les tests précédemment passants sur les calculs d'âge sont préservés avec les mêmes caractéristiques, à savoir qu'il est possible de stipuler un âge pour une période quelconque, et qu'il se répercute ensuite à toutes les autres périodes. Cela se traduit en effet par le calcul préalable d'une date de naissance inférée depuis l'âge donné en entrée et la période; les variables age_en_mois et age sont ensuite calculable pour toute autre période puisque la date de naissance est définie pour ETERNITY.

Elle corrige également #1345 et les tests associés à la PAJE dont le calcul semblait faux en conséquence.

Une conséquence de ce nouveau fonctionnement est que les cas de tests et les payloads devront spécifier uniformément age, age_en_mois ou date_naissance pour tous les individus d'un même vecteur; en effet en l'absence d'une notion de variable partiellement renseignée c'est la seule façon de garantir que les effets de ce mode de calcul sont cohérents. Cette PR n'est pas rétro-compatible.

Par exemple, aujourd'hui, on ne peut plus définir l'âge en mois pour les enfants et la date de naissance pour les parents.

Fixes #1345


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

  • Modifient l'API publique d'OpenFisca France (par exemple renommage ou suppression de variables).
  • Ajoutent une fonctionnalité (par exemple ajout d'une variable).
  • Corrigent ou améliorent un calcul déjà existant.
  • Modifient des éléments non fonctionnels de ce dépôt (par exemple modification du README).

Quelques conseils à prendre en compte :

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

@Morendil
Copy link
Contributor Author

@guillett J'aurais besoin de ton avis sur les tests PAJE impactés par cette PR, et plus largement sur les possibilités qu'il y ait eu des erreurs non détectés sur le nombre d'enfants à charge au sens des AF (cf #1345).

tests/formulas/age.yaml Outdated Show resolved Hide resolved
@sandcha
Copy link
Contributor

sandcha commented Jul 5, 2019

@Morendil Le test ci-dessous (inspiré detests/formulas/paje/paje_2018.yaml) ne passe pas.
Serait-ce parce que, lorsqu'on dit, que ça doit être uniformément spécifié, cela doit être la même variable pour tous les individus ?

- name: PAJE - PPN - Cas N°1
  description: Montant PAJE
  period: 2018-03
  absolute_error_margin: 0.03
  input:
    famille:
      parents: [parent1, parent2]
      enfants: [enfant1]
    foyer_fiscal:
      declarants: [parent1, parent2]
      personnes_a_charge: [enfant1]
    menage:
      personne_de_reference: parent1
      conjoint: parent2
      enfants: [enfant1]
    individus:
      parent1:
        age_en_mois: 38 * 12
        revenu_assimile_salaire:
          2015: 28000
          2016: 28000
          2017: 28000
          2018: 28000
      parent2:
        age: 35
        revenu_assimile_salaire:
          2015: 15000
          2016: 15000
          2017: 15000
          2018: 15000
      enfant1:
        age_en_mois: 2
  output:
    paje: 923.08 / (1 - 0.005) + 92.31 / (1 - 0.005)

Testé aussi pour les combinaisons age_en_mois+date_naissance et age+date_naissance et cela fonctionne.

@Morendil
Copy link
Contributor Author

Morendil commented Jul 5, 2019

@sandcha Oui c'est bien ça. Soit age, soit age_en_mois, soit date_naissance pour tout le monde. On pourra éventuellement améliorer ça en déplaçant les comportements set_input pour qu'ils aient lieu au niveau de SimulationBuilder et pas de Holder, mais pour cela il nous faudrait d'abord l'API Cache. Cf openfisca/openfisca-core#875

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.

Merci pour cette simplification @Morendil 🙌

Reste effectivement à gérer le test en erreur via API Cache.
Néanmoins, actuellement, il ne passe pas non plus sur master.

# Convertit un âge en mois en date de naissance, de sorte que l'âge équivalent se propage à toutes les périodes
def set_input(holder, period, array):
array = holder._to_array(array)
naissance = (datetime64(period.first_month.start).astype('datetime64[M]') - array.astype('timedelta64[M]')).astype('datetime64[D]')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Morendil
Copy link
Contributor Author

Morendil commented Jul 6, 2019

@sandcha Oui le test échoue parce que l'âge et la date de naissance ne sont pas compatibles et comme c'est l'âge qui est saisi en dernier on écrase la date de naissance existante, on n'est donc pas sur une valeur postérieure à avril 2018 mais une en 2017 et on sort donc du cadre d'application des barèmes pour une naissance après avril 2018. Il faudrait supprimer age. Et je pense que c'est prudent d'avoir aussi un avis de la part de la team Mes Aides.

@Morendil
Copy link
Contributor Author

Morendil commented Jul 7, 2019

J'ai poussé un commit avec quelques corrections de tests, le cas qui est vraiment problématique est le cas "PPN - n°1".

Le test suivant est passant sur master, mais échoue sur cette PR; mais il est clair d'après les valeurs que c'est master qui est incohérent:

- name: PAJE - PPN - Cas N°1
  description: Montant PAJE
  period: 2018-06
  absolute_error_margin: 0.03
  input:
    famille:
      parents: [parent1, parent2]
      enfants: [enfant1]
    foyer_fiscal:
      declarants: [parent1, parent2]
      personnes_a_charge: [enfant1]
    menage:
      personne_de_reference: parent1
      conjoint: parent2
      enfants: [enfant1]
    individus:
      parent1:
        age: 38
        revenu_assimile_salaire:
          2015: 28000
          2016: 28000
          2017: 28000
          2018: 28000
      parent2:
        age: 35
        revenu_assimile_salaire:
          2015: 15000
          2016: 15000
          2017: 15000
          2018: 15000
      enfant1:
        date_naissance: 2018-04-10
  output:
    af_nbenf: 0
    age: [38, 35, -9999]
    prestations_familiales_enfant_a_charge: [False, False, False]
    enfant_eligible_paje: [False, False, True]
    paje_naissance: 946.39
    paje_base: 0
    paje_cmg: 0
    paje_prepare: 0
    paje: 941.66 / (1 - 0.005) + 0

Le nombre d'enfants de cette famille n'est pas zéro mais bien un; le bug sur master c'est que l'âge de l'enfant ressort à -9999, c'est cela qui conduit cet individu à ne pas être compté comme un enfant à charge dans la famille (mais malgré cela éligible à la PAJE quand même!). Le calcul de la PAJE de base effectué est donc faussé.

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Je confirme qu'il reste encore un test qui échoue.

@bonjourmauko
Copy link
Member

Rebased

@Morendil
Copy link
Contributor Author

@sandcha @maukoquiroga J'ai corrigé les tests PAJE "mécaniquement" c'est-à-dire sans vérifier dans le détail la logique métier que je ne maîtrise pas assez, en partant du principe que les valeurs calculées actuellement par OpenFisca sur la base d'un âge cohérent (0 et plus -9999) reflètent effectivement le droit. C'est peut-être une supposition risquée. Je n'ai pas d'autres changements à apporter à cette PR.

@guillett si tu as le temps de revoir ça sous l'angle métier ça me rassurerait là-dessus.

@bonjourmauko bonjourmauko dismissed their stale review November 12, 2019 13:02

Les corrections ont été ajoutées depuis

@bonjourmauko
Copy link
Member

Rebased

@bonjourmauko bonjourmauko changed the title Modifie les modalités de calcul de l'âge Corrige les modalités de calcul de l'âge Dec 12, 2019
@sandcha sandcha self-assigned this Jan 13, 2020
@sandcha
Copy link
Contributor

sandcha commented Jan 13, 2020

Rebase en cours.

)
)
# Convertit un age en date de naissance, de sorte que l'âge équivalent se propage à toutes les périodes
def set_input(holder, period, array):
Copy link
Contributor

Choose a reason for hiding this comment

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

Cette PR résoud le calcul de l'âge 🙌
Toutefois, comme vu plus haut, elle supposerait de contraindre l'expression des âges de la population d'une simulation à une seule variable parmi age et age_en_mois.

Ceci supposerait d'informer l'usager du risque d'obtention de valeurs fausses en cas d'inputs mixtes age/age_en_mois et peut affecter un usage tel que celui fait par mes-aides. 😕

Il me semble qu'on aurait moyen de conserver des inputs mixtes avec age et age_en_mois et faire passer le test proposé dans l'issue à l'origine de cette PR. Dans ce cas, nous aurions besoin :

  • De faire appel à des mises à jour multiples sans perte de donnée de date_naissance puisque cette variable peut-être affectée par age et age_en_mois,
  • D'autoriser des mises à jour multiples d'une variable en ETERNITY puisque c'est la périodicité de date_naissance

Pour le premier point, nous pourrions utiliser des masques.
Voici un draft pour age qui se base sur le code de cette PR :

def set_input(holder, period, array):
        age = holder._to_array(array)
        age[age == -9999] = 48
        naissance_par_age = (datetime64(period.first_month.start).astype('datetime64[M]') - (age * 12).astype('timedelta64[M]')).astype('datetime64[D]')
        
        naissance_connue = holder.simulation.calculate('date_naissance', period)
        
        naissance_default_value = datetime64('1970-01-01')
        naissance_inconnue = ma.masked_where(naissance_connue == naissance_default_value, naissance_connue)
        index_naissance_inconnue = np.where(naissance_inconnue.mask)

        naissance_connue[index_naissance_inconnue] = naissance_par_age[naissance_inconnue.mask]
        holder.simulation.set_input('date_naissance', period, naissance_connue)

Pour le deuxième point, nous aurions besoin de mettre à jour la gestion des valeurs dans les holders. Ceci peut être facilité par la PR openfisca/openfisca-core#891 que je vous propose de revoir avant celle-ci.

@bonjourmauko
Copy link
Member

Hello @sandcha, je vois que cette PR est acceptée, est-ce que l'on peut la merger ?

Juste pour être sûr ...

Le cas échéant cela sera bien de changer son statut et de spécifier dans la review ce qu'il faudrait pour que ce soit le cas.

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.

Clarification du statut de cette PR : Elle supposerait d'informer l'usager du risque d'obtention de valeurs fausses en cas d'inputs mixtes age/age_en_mois. Ce risque me semble trop élevé pour merger le code en l'état.

@bonjourmauko bonjourmauko added this to the 2020/T2 milestone Feb 16, 2020
@bonjourmauko
Copy link
Member

bonjourmauko commented Feb 22, 2020

@sandcha c'est-à-dire que, en l'état, et en absence de l'API cache, ces changements ne sont pas rétro-compatibles, que le risque de confusion rend le bénéfice(x) de cette PR plus faible que son coût(x), et que donc on peut a) soit la fermer sans merger, b) soit indiquer explicitement une dépendance à l'API cache ?

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.

Corriger le calcul des âges
3 participants