Remplace les méthodes .period par une instanciation de la classe Period()

[eraviart]

• Amélioration technique.
• Périodes concernées : toutes.
• Zones impactées : model.
• Détails :
  • Depuis la version 37.0.0, openfisca-core n'accepte plus la méthode "period()". Cette modification remplace tous les appels de cette méthode par des instanciations de la classe Period.

[eraviart]

Je ne suis pas totalement fan de la nouvelle syntaxe qui remplace la méthode period(), mais bon.

[bonjourmauko]

Hello @eraviart , la méthode a tout simplement disparu car elle faisait râler les linters. Par contre on perde l'élégance du chaining. Pourrais-tu jeter un coup d'oeil à ma proposition sur core pour combler ce vide? 🙂

[eraviart]

@maukoquiroga Les lignes que j'ai remplacées partent d'un instant pour construire une période. Or il me semble que la solution avec la méthode this partent d'une période pour construire une autre période.

Pour faire un test, j'ai pris le premier diff de cette PR :
period.start.offset('first-of', 'year').period('month', 11)
qui a été pour l'instant remplacé par Period(('month', period.start.offset('first-of', 'year'), 11))

Qu'est ce que cela deviendrait avec la méthode this ?

[bonjourmauko]

Hello @eraviart ! En effet, period.this c'est l'équivalent à this_year, first_month, etc.

Pour Period(('month', period.start.offset('first-of', 'year'), 11)), tu as deux options:

1. Si tu veux dire à partir du début d'année, onze mois après, il y a un peu plus bas la méthode come.

Normalement, cela devrait en être l'équivalent : period.this(YEAR).come(MONTH, 11).

2. Si tu veux toute la période, et pas seulement le 11e mois:

period.this(YEAR).until(MONTH, 11)

Peut-être que la méthode this pourrait s'appeler first pour plus de clarté ?

Tu as aussi deux autres pour le passé:

3. period.ago(MONTH, 6) (il y a six mois)

4. period.last(MONTH, 3) (les trois derniers mois)

Le seul point qui posait souci est que la méthode period soit ratachée à Instant (cyclic imports), mais tu as toujours la méthode period du module periods.period(instant) qui est l'équivalent de la méthode dépreciée:

periods.period(instant).this(YEAR).come(MONTH, 11)

Enfin tout cela ce n'est qu'un wrap d'offset qui marche comme toujours, mais pour permettre de créer des offset plus idiomatiques sans changer le core. Par exemple maintenant que l'on rajoute des semaines, on pourra faire period.last(WEEK, 52).offset(1) (le mardi d'il y a 52 semaines) de manière plus flexible.

[eraviart]

J'ai cru comprendre que le problème de la suppressions de instant.period() était dû à des dépendances circulaires entre instants et periods. Ne serait-il pas possible de, par exemple, fusionner periods et instants dans le même fichier Python pour régler ce problème et conserver ainsi instant.period() ?

[bonjourmauko]

Hello @eraviart

J'ai cru comprendre que le problème de la suppressions de instant.period() était dû à des dépendances circulaires entre instants et periods. Ne serait-il pas possible de, par exemple, fusionner periods et instants dans le même fichier Python pour régler ce problème et conserver ainsi instant.period() ?

Ce n'est pas le problème mais l'un de problèmes, et qui n'est pas spécifique aux Period et Instant mais un peu partout dans le core : Enum et EnumArray, Entity, Holder, Variable, Population et TaxBenefitSystem etc.

De ce fait, une proposition de re-fusionner Period et Instant dans un même fichier doit être comprise comme une proposition de fusionner toute classe où l'on éprouverait ce problème. Je ne suis pas favorable à cette approche.

Par contre, si par exemple les périodes ou les instants changaient pour avoir moins de logique et devenir plus des containers d'information, on pourrait le fusionner, mais cela ne justifierait pas encore amha instant.period().

La proposition que j'ai faite dans openfisca/openfisca-core#1138 va dans le sens de ce que j'ai cru être le design des Period et Instant :

1. Rendre plus explicite qu'une Period est un object composé entre autres d'un Instant.
2. Rendre tout de même les deux Period et Instant plus container d'information.
3. Rendre plus puissants les helpers déjà existants periods.period et periods.instant pour créer des instances.
4. Extraire toute la logique commune de parsing dans un module appart.

Dans ce scénario - qui n'est qu'un possible -, instant.period() reste problématique puisque c'est une class plus abstraite qui produit des instances d'une class plus concrète. Cela crée de problèmes au-delà des circular imports.

Par contre, si l'on disait que Period et Instant sont au même niveau, comme on fait avec datetime.date avec la méthode date() (que je propose devenir méthode pour cette raison), là oui on pourrait avoir instant.period().

Si cela te convient, je te propose qu'on merge cette pull request et que l'on continue cette discussion directement sur openfisca/openfisca-core#1138.

[sandcha]

Merci @eraviart pour cette migration 🙌

Un détail que nous pouvons mettre à jour dans le CHANGELOG de cette PR : la version v37 de core a été dépubliée au bénéfice de la v38.0.2 comme indiqué dans le CHANGELOG openfisca-core.

[sandcha]

Une limite peut-être à notre syntaxe qui existait précédemment à cette PR :

Ici Period(('month', period.start.offset('first-of', 'year'), period.start.month - 1)) ne fonctionne que parce que period.start.month > 1.

Sinon, on obtient une AssertionError pas très claire sur la démarche à suivre. Extrait de l'erreur avec l'ancienne syntaxe :

  File "/Users/sch/.local/share/virtualenvs/fr1961/lib/python3.7/site-packages/openfisca_core/periods/instant_.py", line 97, in period
    assert isinstance(size, int) and size >= 1, 'Invalid size: {} of type {}'.format(size, type(size))
AssertionError: Invalid size: 0 of type <class 'int'>

[sandcha]

PR testée dans un script vérifiant l'égalité des résultats entre l'ancienne et la nouvelle syntaxe (ce qui m'a semblé plus simple que de lire et l'ancienne et la nouvelle syntaxe 😊 😬 ).

Ce mini script nous fait aussi un bilan des syntaxes utilisées dans openfisca-france pour ce sujet alors je partage au cas où :

from openfisca_core.periods import Instant, Period, YEAR, MONTH


first_jan = Instant((2023, 1, 1))
first_feb = Instant((2023, 2, 1))

period_mois = Period((MONTH, first_jan, 1))
period_mois_superieur_un = Period((MONTH, first_feb, 1))
period_annee = Period((YEAR, first_jan, 1))


assert period_mois.start.offset('first-of', 'year').period('month', 11) == Period(('month', period_mois.start.offset('first-of', 'year'), 11))

assert period_mois.start.offset('first-of', 'year').period('month', period_mois.start.month) == Period(('month', period_mois.start.offset('first-of', 'year'), period_mois.start.month))

assert period_mois_superieur_un.start.offset('first-of', 'year').period('month', period_mois_superieur_un.start.month - 1) == Period(('month', period_mois_superieur_un.start.offset('first-of', 'year'), period_mois_superieur_un.start.month - 1))

assert period_mois.start.offset('first-of', 'month').offset(-11, 'month').period('month', 11) == Period(('month', period_mois.start.offset('first-of', 'month').offset(-11, 'month'), 11))

ecart_avec_2000 = period_annee.start.offset('first-of', 'year').year - 2000
assert period_annee.start.offset('first-of', 'year').period('year').offset(-ecart_avec_2000) == Period(('year', period_annee.start.offset('first-of', 'year'), 1)).offset(-ecart_avec_2000)

assert period_mois.start.period('year').offset(-1).offset(-1, 'month') == Period(('year', period_mois.start, 1)).offset(-1).offset(-1, 'month')

assert period_mois.start.period('year').offset(-1) == Period(('year', period_mois.start, 1)).offset(-1)

assert period_mois.start.offset('first-of', 'month').period('month') == Period(('month', period_mois.start.offset('first-of', 'month'), 1))

assert period_mois.start.offset('first-of', 'year').period('year') == Period(('year', period_mois.start.offset('first-of', 'year'), 1))

assert period_mois.first_month.start.period('month', 3).offset(-3) == Period(('month', period_mois.first_month.start, 3)).offset(-3)

assert period_mois.start.period('month', 3).offset(-3) == Period(('month', period_mois.start, 3)).offset(-3)

assert period_mois.start.period('month').offset(-1) == Period(('month', period_mois.start, 1)).offset(-1)

assert period_mois.start.offset('first-of', 'year').period('year', 2).offset(-1) == Period(('year', period_mois.start.offset('first-of', 'year'), 2)).offset(-1)

assert period_annee.start.offset('first-of', 'year').offset(9, 'month').period('month') == Period(('month', period_annee.start.offset('first-of', 'year').offset(9, 'month'), 1))
assert period_annee.start.offset('first-of', 'year').offset(11, 'month').period('month') == Period(('month', period_annee.start.offset('first-of', 'year').offset(11, 'month'), 1))

assert period_mois.start.period('month', 6).offset(-6) == Period(('month', period_mois.start, 6)).offset(-6)

assert period_annee.start.period('year').offset(-1) == Period(('year', period_annee.start, 1)).offset(-1)

assert period_mois.start.period('month').offset('first-of') == Period(('month', period_mois.start, 1)).offset('first-of')