Corrige zone_logement_social pour retourner un vecteur

[bonjourmauko]

Fixes #1394

• Amélioration technique.
• Périodes concernées : toutes.
• Zones impactées : model/prestations/logement_social.py.
• Détails :
  • La formule zone_logement_social retournait un scalar numpy.ndarray[int].
  • Corrige zone_logement_social pour retourner un vecteur numpy.ndarray[bool].
  • Note : à partir de Numpy 1.18, l'utilisation de select avec un valeur non bool est dépréciée.

---

Ces changements :

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

---

Quelques conseils à prendre en compte :

• Jetez un coup d'œil au guide de contribution.
• Regardez s'il n'y a pas une proposition introduisant ces mêmes changements.
• Augmentez le numéro de version dans setup.py.
• Mettez à jour le CHANGELOG.md.
• Assurez-vous de bien décrire votre contribution, comme indiqué ci-dessus

[Morendil]

Je recommanderais plutôt de dépublier la version de Core qui a causé la régression, puis d'analyser ce qui est nécessaire pour la compatibilité avec Numpy. Je trouve aussi qu'introduire Mypy n'est pas pertinent car en dehors du scope de cette PR.

[benjello]

@maukoquiroga : j'ai corrigé le test qui passe maintenant avec la version de core master et la version de la PR openfisca/openfisca-core#923.

Je te laisse choisir avec @Morendil la stratégie adéquate.

[bonjourmauko]

Merci @Morendil

Je recommanderais plutôt de dépublier la version de Core qui a causé la régression.

👍

Puis d'analyser ce qui est nécessaire pour la compatibilité avec Numpy.

Tu dis appart de l'utilisation de numpy.select ?

Je trouve aussi qu'introduire MyPy n'est pas pertinent car en dehors du scope de cette PR.

La première chose que j'ai faite pour cibler le problème est de faire une hypothèse de signature.

Je trouve donc au moins les annotations de type utiles, même si le type checking n'est effectivement pas nécessaire (or j'ai l'ai utilisé dans la cadre de cette PR).

Tu dis d'enlever MyPy lui-même ou les annotations aussi ?

[bonjourmauko]

Merci @benjello ! Si l'attendu de sortie est toujours un vector et pas un scalar, effectivement c'est la solution pertinente.

Peut-on assumer ça ? Si oui je change les PR correspondantes pour refléter ce fait.

@guillett @Morendil

[bonjourmauko]

@Morendil en suivant tes conseils, j'ai fait une analyse de compatibilité OpenFisca Core avec Numpy 1.18 : openfisca/openfisca-core#924.

Bien que le code concerné par le changement utilise quasi-exclusivement numpy.select, il y a des bouts du code pas testés. Le changement n'est, donc, pas safe.

TODO :

• Dépublier la version de Core qui a causé le problème
• Faire un analyse de compatibilité sur Core
• Faire un analyse de compatibilité sur France

Au-delà de ces considérations, cette PR reste d'actualité, j'actualise juste la description.

Merci!

[Morendil]

A mon sens ce n'est pas une question de scalaire ou de vecteur; la version 1.18 de Numpy a supprimé la possibilité d'utiliser des entiers dans les vecteurs fournis à select et impose désormais d'utiliser des vecteurs correctement typés comme booléens.

C'est moins la compatibilité de Core avec Numpy 1.18 qui me pose souci que la compatibilité de France, et par extension des autres pays.

Cela me mettait mal à l'aise de savoir que dans beaucoup d'endroits dans France on utilise des sommes ou des multiplications pour manipuler des valeurs logiques; sachant cela je me serais attendu à ce que le problème soit beaucoup plus étendu.

Je suis agréablement surpris que la formule de zone_logement_social soit la seule affectée par ce changement, c'est une bonne nouvelle. Et pour les auteurs de formules il serait utile de documenter "comment réaliser des combinaisons complexes de vecteurs de valeurs logiques".

A mon avis il vaut mieux utiliser np.logical_or.reduce plutôt que sum et ensuite caster le résultat vers un dtype=bool: l'idée c'est bien de se dire que les vecteurs sont typés et qu'il faut les manipuler avec les primitives Numpy qui correspondent à leur type.

[Morendil]

Utiliser np.logical_or.reduce ; les array depcom == … sont de type booléen, c'est dommage de les forcer en valeurs entières puis refaire l'opération inverse.

in_paris_communes_limitrophes = np.logical_or.reduce([depcom == commune_proche_paris
                for commune_proche_paris in paris_communes_limitrophes])

…mais d'ailleurs est-ce que Numpy ne propose pas une primitive pour tester l'appartenance d'une valeur à un ensemble ou à une liste ?

[bonjourmauko]

Merci ! Si j'ai bien compris

np.logical_or.reduce

est l'équivalent de

np.any_(foo, axis = 0)

[Morendil]

…oui mais comme je le disais plus haut il est encore préférable d'utiliser np.isin que je découvre à l'instant dans Numpy:

>>> import numpy as np
>>> test_values = [3,5,7]
>>> a = np.array([1,2,3,4,5,6,7,8,9,10])
>>> np.logical_or.reduce([a == v for v in test_values])
array([False, False,  True, False,  True, False,  True, False, False, False])
>>> np.isin(a,test_values)
array([False, False,  True, False,  True, False,  True, False, False, False])

…soit dans notre cas:

np.isin(depcom, paris_communes_limitrophes)

[bonjourmauko]

Soit l'exemple complet :

in_paris_communes_limitrophes = isin(depcom, paris_communes_limitrophes)
in_idf = isin([True], startswith(depcom, departements_idf))

[Morendil]

in_idf = isin([True], startswith(depcom, departements_idf))

Je pense que ce n'est pas correct. isin va renvoyer un vecteur de la même taille que son premier argument, donc un seul élément dans la formulation ci-dessus, même si le vecteur depcom a une taille différente de 1.

Là encore il vaut mieux utiliser une opération Numpy (vectorisée) sur les chaines, à la place de l'opération correspondante en Python; en fait ce qu'on veut c'est les 2 premiers caractères de depcom, qui correspondent au département. C'est numpy.char.ljust.

>>> a = np.array(["93451","75001","45001"])
>>> np.isin(np.char.ljust(a,2),["93","75"])
array([ True,  True, False])

Donc

in_idf = np.isin(np.char.ljust(depcom,2), departements_idf)

Tout ça me fait penser au passage à openfisca/openfisca-core#673 - on a eu tendance à sous-estimer l'importance pour les utilisateurs de comprendre le calcul vectoriel en général et Numpy en particulier.

Pour cette raison d'ailleurs je suis plus favorable à l'import en bloc import numpy as np (ou même comme recommandé par Matti import numpy) de préférence à l'import sélectif from numpy import (isin, char, …) de façon à mettre en évidence les portions du code qui appellent les primitives Numpy.

[bonjourmauko]

in_idf = np.isin(np.char.ljust(depcom,2), departements_idf)

👍

Pour cette raison d'ailleurs je suis plus favorable à l'import en bloc import numpy as np (ou même comme recommandé par Matti import numpy) de préférence à l'import sélectif from numpy import (isin, char, …)

C'est pour cela que j'essaie d'éviter les imports globaux :

from openfisca_france.model.base import *

Concernant l'import du module vs. l'import selectif, après lire cet article, je suis d'accord.

[benjello]

JE suis aussi pour virer les import globaux aussi souvent que possible

[benjello]

@maukoquiroga : je garderais l'ancienne convention de passer par des alias raccourcis (import numpy.isin as isin_ par exemple pour merger cette PR. Et surtout merger le calcul du taux marginal et de la tranche qu'attends @PhunkyBob.

[benjello]

Voir mon commentaire (désolé).

[bonjourmauko]

@benjello OK pour garder la convention et continuer la discussion sur #1398