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

Ajoute un epsilon aux delta temps pour fiabiliser le calcul des âges #1166

Merged
merged 3 commits into from
Oct 31, 2018

Conversation

Morendil
Copy link
Contributor

  • Amélioration technique.
  • Périodes concernées : toutes.
  • Zones impactées : openfisca_france/model/prelevements_obligatoires/impot_revenu/ir.py.
  • Détails :
    • Fiabilise le calcul de l'âge en années et en mois

Fixes #782 (selon la suggestion de @MattiSG)

Pas de tests unitaires, j'arbitre consciemment pour l'augmentation légère de la dette technique en faveur de la réduction du WIP.


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

@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.

Merci @Morendil

Il me semble que sans tests ni de la documentation, cela ajoute en effet une dette technique importante.

Vu que c'est un cas qui n'est pas isolé, je crois qu'un test est nécessaire et bienvenu.

@@ -93,7 +93,8 @@ def formula(individu, period, parameters):
)

date_naissance = individu('date_naissance', period)
return (datetime64(period.start) - date_naissance).astype('timedelta64[Y]')
epsilon = timedelta64(1)
return (datetime64(period.start) - date_naissance + epsilon).astype('timedelta64[Y]')
Copy link
Member

Choose a reason for hiding this comment

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

C'est un problème qu'on a déjà rencontré dans d'autres modèles, et qu'on a résolu d'une façon différente.
On a dû oublié de le corriger dans france à l'époque 😕...

Il me semble avoir testé le epsilon, mais que ça restait problématique dans certains cas. Je ne me souviens plus exactement pourquoi, mais normalement le fix du country template est robuste 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je tente le coup. Effectivement ça marche, mais ça me semble moins général: le fix du epsilon était également utilisé dans cette PR pour age_en_mois qui rencontre le même type d'erreur, et adapter la solution de country_template pour age_en_mois est nettement moins séduisant. Il ne s'agit plus de transposer "divise par les années" en "divise par les mois", il faut ajouter le mois de la période et soustraire le mois de naissance mais seulement si l'anniversaire est passé… ou un truc comme ça, c'est compliqué à faire de tête et donc il faudrait effectivement le faire avec des TU, et faire des TU en YAML c'est lourdingue.

Même conclusion donc que pour ma conversation précédente avec Mauko: cette PR est ma meilleure idée, je ne sais pas aller plus loin d'une façon qui me semble satisfaisante. Je vous laisse la main.

Copy link
Member

@fpagnoux fpagnoux Oct 19, 2018

Choose a reason for hiding this comment

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

faire des TU en YAML c'est lourdingue.

Je ne sais pas si tu l'as déjà rencontré, mais il y a un format de test YAML simplifié qui ne force pas à définir les entités

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@fpagnoux fpagnoux left a comment

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

J'ai ajouté un TU que j'ai bien vérifié comme non passant dans master, mais je ne sais toujours pas transposer l'exemple pointé par @fpagnoux de façon satisfaisante dans France avec son age_en_mois. S'appuyer sur les primitives de manipulation des dates me semble plus performant et moins error-prone que l'implémenter nous-mêmes de façon très spécifique. Peut-on merger pour l'instant cette solution qui passe les anciens tests plus un nouveau, et la revisiter plus tard quand nous aurons peut-être plus de connaissances, éventuellement via d'autres pays ?

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.

Pour moi c'est bon, merci @Morendil.

@fpagnoux
Copy link
Member

Je n'ai pas trouvé de test qui font échoué la formule, donc ok pour moi

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.

3 participants