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

Recode la taxe d'habitation #1320

Merged
merged 69 commits into from
Jun 13, 2019
Merged

Recode la taxe d'habitation #1320

merged 69 commits into from
Jun 13, 2019

Conversation

bfabre01
Copy link

@bfabre01 bfabre01 commented May 6, 2019

  • Amélioration de modélisation - changement majeur
  • Périodes concernées : toutes.
  • Zones impactées :
    • prelevements_obligatoires/taxe_habitation
    • prelevements_obligatoires/isf
    • mesures
  • Détails :
    • Re-code la taxe d'habitation (TH) : on code la TH des communes et EPCI (Etablissement Public de Coopération Intercommunale) due au titre de la résidence principale à partir de la législation 2017, et jusqu'à la législation 2019. Cette PR intègre les barèmes locaux votés par les communes et EPCI (taux de taxation, taux d'abattements) de 2017.
    • Paramètres supprimés:
      - cotsoc/gen/plaf_th_1
      - cotsoc/gen/plaf_th_1_dom
      - cotsoc/gen/plaf_th_1_guy
      - cotsoc/gen/plaf_th_supp
      - cotsoc/gen/plaf_th_supp1_dom
      - cotsoc/gen/plaf_th_supp1_guy

@bfabre01 bfabre01 requested a review from claireleroy May 6, 2019 13:07
@sandcha sandcha requested review from Morendil and sandcha May 6, 2019 14:27
@sandcha
Copy link
Contributor

sandcha commented May 6, 2019

Merci @bfabre01 pour cette contribution !
Comme elle est conséquente, @Morendil et moi sommes intéressés par sa revue. Avez-vous des contraintes de temps particulières ?

@bfabre01
Copy link
Author

bfabre01 commented May 6, 2019

@sandcha @Morendil : Merci ! Si ça peut être reviewé d'ici 15 jours c'est top ! Comme j'ai posté sur slack, je n'arrive pas à comprendre l'erreur dans la section build des tests... Si vous avez une explication, je suis preneur !

@bfabre01
Copy link
Author

bfabre01 commented May 6, 2019

Grosso modo, cette PR est assez longue en lignes de codes, mais ne devrait pas poser trop de problèmes je pense. Il s'agit juste d'ajouter la TH, qui n'agit pas sur les autres dispositifs socio-fiscaux simulés dans OF France. Donc, la revue devrait surtout être du check de législation ou de forme.

@bfabre01
Copy link
Author

bfabre01 commented May 7, 2019

@Morendil @sandcha: je viens d'ajouter les barèmes communaux et intercommunaux de la TH pour 2017 sous format csv (avant, j'avais juste créé des fichiers yaml avec les barèmes de deux communes pour faire les tests). Je suis preneur de retour sur le code qui récupère les infos de ce fichier csv, car il n'est je pense clairement pas optimisé ! @fpagnoux, si toi aussi tu peux y jeter un oeil rapide, ça serait top ! Notamment pour voir si un tel code ne serait pas trop inefficace si on doit le faire tourner sur des grosses bases de données !

@sandcha
Copy link
Contributor

sandcha commented May 7, 2019

Deux compléments d'information pour la revue :

@@ -0,0 +1,48 @@
/***************************************************************************************/
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this file should be commited in this repository

Copy link
Author

Choose a reason for hiding this comment

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

Pas sûr non plus. Je l'ai juste ajouté par transparence sur la génération du csv. Mais je peux l'enlever. @sandcha @Morendil , qu'en pensez-vous ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok pour le conserver mais ce serait alors dans openfisca-france/openfisca_france/scripts.
Et dans ce cas, pour le rendre utilisable, peux-tu indiquer la version de stata employée (aka, le nécessaire à avoir dans l'environnement pour l'appeler, librairies...) ainsi que la commande pour l'exécuter ?

Copy link
Contributor

Choose a reason for hiding this comment

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

En as-tu une version en Python pour accroître ses chances d'être mis à jour ?

Copy link
Author

Choose a reason for hiding this comment

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

@sandcha : j'ai ajouté la version de stata nécessaire pour lancer le programme. Pour l'emplacement, je trouve qu'il est mieux de garder le programme près du fichier .csv sinon, peut-être qu'il n'est pas évident de savoir que ce programme existe lorsqu'on consulte le csv.

@sandcha
Copy link
Contributor

sandcha commented May 13, 2019

@bfabre01 Il semble qu'il y ait des variables supprimées et renommées. C'est donc non rétro-compatible.
Peux-tu indiquer la liste des variables renommées et supprimées stp ?
De même pour les paramètres en indiquant simplement le noeud parent affecté ?

Copy link
Contributor

@Morendil Morendil left a comment

Choose a reason for hiding this comment

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

Premières remarques à la suite d'une relecture avec @sandcha, on continue demain

return exon


class valeur_locative_cadastrale_brute(Variable):
Copy link
Contributor

Choose a reason for hiding this comment

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

Cette variable étant le seul input spécifique à la nouvelle TH, ça serait utile de l'avoir en début de fichier.

Copy link
Author

Choose a reason for hiding this comment

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

Fait.

reference = "art. 1414 C du CGI"
definition_period = YEAR

def formula_2018_01_01(menage, period, parameters):
Copy link
Contributor

Choose a reason for hiding this comment

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

Une remarque de forme: je trouve la formule un peu difficile à lire, l'idéal serait d'utiliser des sauts de ligne pour aérer, entre les parties qui font un même travail; et peut-être regrouper ce qui va ensemble, par exemple faire d'abord le dégrèvement et ensuite le dégrèvement dégressif.

Copy link
Author

@bfabre01 bfabre01 May 13, 2019

Choose a reason for hiding this comment

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

Fait. Par contre, le dégrèvement et le dégrèvement dégressif sont très liés. C'est donc plus simple je pense de mettre les lignes de calculs de ces deux dispositifs ensemble.

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 @bfabre01 !
Voici un premier retour de forme (hors script de lecture du csv).

taux_th_commune = menage('taux_th_commune', period)
taux_th_epci = menage('taux_th_epci', period)
if taux_th_commune + taux_th_epci == 0:
log.error(("Attention, il n'y a pas de taux de taxe d'habitation défini pour la commune indiquée."))
Copy link
Contributor

Choose a reason for hiding this comment

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

Supprimer le message d'erreur et préférer une liste de taux complète.

Copy link
Author

Choose a reason for hiding this comment

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

@sandcha : en fait, les taux de 2000 ne sont pas disponibles, d'où le fait que j'ai mis un message d'erreur. Même si on n'a pas ces taux, je les ai introduit dans le code (1) pour coller à la législation (2) pour qu'il soit possible de faire une simul pour un cas particulier dont on connait le taux en 2000.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Ca nous donne donc un équivalent de valeur par défaut pour les taux manquants.

Il ne reste donc qu'à supprimer le message de log (par cohérence avec le reste du comportement d'openfisca qui passe par des valeurs par défaut mais ne log qu'en cas de crash).

default_value = False
entity = FoyerFiscal
label = u"Condition de revenu fiscal de référence pour l'éxonération à l'échelle du foyer fiscal"
reference = "BOI-IF-TH-10-50-30"
Copy link
Contributor

Choose a reason for hiding this comment

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

Utiliser une URL fixe du site bofip.impots.gouv.fr ?

Copy link
Author

Choose a reason for hiding this comment

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

Fait.

return degrev * elig_degrev + degrev_degressif * elig_degrev_degressif


class taxe_habitation_commune_epci(Variable):
Copy link
Contributor

@sandcha sandcha May 13, 2019

Choose a reason for hiding this comment

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

Conserver le nom de taxe_habitation pour la variable de plus haut niveau dans le calcul ? Le libellé peut toujours indiquer plus de précision.

Copy link
Author

Choose a reason for hiding this comment

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

Fait.

class taxe_habitation_commune_epci(Variable):
value_type = float
entity = Menage
label = u"Taxe d'habitation de la commune et de l'EPCI, frais de gestion inclus"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label = u"Taxe d'habitation de la commune et de l'EPCI, frais de gestion inclus"
label = u"Taxe d'habitation de la commune et de l'EPCI (Établissement Public de Coopération Intercommunale), frais de gestion inclus"

Copy link
Author

Choose a reason for hiding this comment

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

Fait.

value_type = float
entity = Menage
label = u"Dégrèvement d'office de la taxe d'habitation"
reference = "art. 1414 C du CGI"
Copy link
Contributor

Choose a reason for hiding this comment

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

Cet article 1414 C est antérieur à 2018. Mettre à jour la référence pour qu'elle indique la modification applicable depuis 2018 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Et remplacer toutes les références en chaînes de caractère par le lien légifrance/fixe ?

Copy link
Author

Choose a reason for hiding this comment

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

Fait.

@@ -0,0 +1,8 @@
description: Plafond de revenu fiscal de référence pour le dégrèvement d'office à taux plein - 1ère part
Copy link
Contributor

Choose a reason for hiding this comment

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

Tous ces paramètres ont le même préfixe plaf_rfr...
Utiliser ce préfixe en nom du noeud parent ?

Voire, regrouper quelques paramètres dans un même fichier yaml pour en faciliter la lecture.

Copy link
Author

Choose a reason for hiding this comment

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

Fait.

openfisca_france.__name__,
'assets/taxe_habitation/parametres_th_{}.csv'.format(year),
) as csv_file:
if sys.version_info < (3, 0):
Copy link
Contributor

Choose a reason for hiding this comment

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

Python 2 n'étant plus maintenu, supprimer le code qui assure la rétro-compatibilité pour faciliter la maintenance de l'ensemble 🙂

Copy link
Author

Choose a reason for hiding this comment

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

Fait

@bfabre01
Copy link
Author

@Morendil @sandcha : merci pour vos commentaires ! J'ai fais les modifs.

@Morendil
Copy link
Contributor

Morendil commented May 20, 2019

@bfabre01 Je continue la revue. J'ai des réserves sur axe_habitation_import_baremes_locaux.py.

Ce fichier fait 500+ lignes mais pour l'essentiel c'est du code qui décline une même opération: lire un CSV, extraire une colonne, l'affecter à une variable globale Python pour la mettre à disposition d'une variable OpenFisca.

L'opération de lecture du fichier est répétée autant de fois qu'il y a de variables concernées, ça me semble un peu préoccupant en termes de performance autant que de factorisation du code.

Il vaut mieux réaliser le "preload" une seule fois et affecter l'ensemble des colonnes à un Dict unique (qui peut être une globale), de façon paresseuse (ne pas ré-affecter si le Dict est déjà initialisé). De cette façon il n'y aurait qu'une seule copie de la fonction preload. (Ce preload peut d'ailleurs être mis en commun avec celui qui est dans aides_logement en le généralisant un peu.)

Le isinstance(code_INSEE_commune_cell, str) est superflu, nous n'avons pas besoin de supporter Python 2; on peut supposer que le fichier est encodé en UTF8 et que les chaînes sont bien des str.

Plutôt que

montant = fromiter((default_value for code_INSEE_commune_cell in code_INSEE_commune), dtype = float)

utiliser

np.repeat(default_value, size(code_INSEE_commune))

Il me semble qu'on peut aussi éviter l'itération dans le cas où le CSV est présent:

indices = np.unique(code_INSEE_commune, return_inverse=True)[1]
values = np.array(list(abt_condition_modeste_by_epci.values()))
montant = values[indices]

mais je ne sais pas ce qui est préférable en termes de performance, ça peut être intéressant de faire des essais. Ou bien @benjello doit pouvoir te dire d'après son expérience.

@benjello
Copy link
Member

@Morendil @bfabre01 : mon expérience ce limite à l'utilisation du preload pour éviter de relire le fichier à chaque calcul et de faire un cahce avec l'utilisation d'une varaible globale;
Je rejoisn @Morendil sur le fait de lire toutes les colonnes en une seule fois car de tout e façon on est obligé d'utiliser donc de lire tous ces paramètres pour le calcul de la TH.

@sandcha
Copy link
Contributor

sandcha commented May 20, 2019

@bfabre01 Nous avons remarqué qu'il manquait des tests à TH supérieure à différente de zéro et que le taux était souvent fixé.

Peux-tu comparer les résultats de ton modèle avec ceux de cette application pour faire varier plus de points d'entrée en test (typiquement, valeur locative/rfr/localisation) ?

L'idée serait d'intégrer ces tests à la PR si un écart (> 2€) est détecté.

@bfabre01
Copy link
Author

@sandcha @Morendil : merci pour vos retours.
Pour le preload. Je suis d'accord, c'est inefficace. J'avais essayé de tout regrouper en une seule opération, mais il y avait un bug que je ne comprenais pas, et qui n'était pas présent quand je séparais les choses. Je vais essayer de faire mieux. Mais si vous voyez une solution assez évidente pour vous et rapide, n'hésitez pas à l'implémenter (mes connaissances en python touchent leurs limites).
@sandcha : quand tu dis un test où TH >0, tu penses à des cas où il y aurait des restitutions de TH c'est bien ça ? Comme pour l'IR où les crédits d'impôts peuvent être > à l'impôt avant crédits ? Si c'est le cas, c'est normal qu'il n'y ait pas de tels tests : à ma connaissance, il ne peut pas y avoir de restitution de TH.
Je n'ai en effet pas fait varier les taux de taxation et les abattements. Mais j'ai prix le cas d'une commune qui avait voté une valeur >0 de tous les abattements modélisés. Et fait varier la valeur locative et le RFR de sorte à avoir, en un minimum de tests, l'ensemble des configurations possibles en termes valeurs relativesde RFR/val locative/taux. Si vous voyez une combinaison que j'aurais manqué, dites-le moi. Je vais regarder l'application citée.

@Morendil
Copy link
Contributor

@bfabre01 On ne parlait pas des restitutions mais simplement une TH à payer non nulle.

@bfabre01
Copy link
Author

@Morendil @sandcha : j'ai fait quelques modifs et comparé tous mes résultats de tests yaml avec l'appli de Marion Paclot. Tout est ok (sauf un cas, pour lequel je viens de contacter Marion, car je pense qu'on est bon de notre côté). Donc, niveau test, je pense que c'est ok. Pour le chargement des paramètres, ça m'arrangerait si vous pouviez le faire. Cela vous prendrait beaucoup moins de temps, et permettrait de fermer cette (longue) PR assez vite. C'est ok pour vous ?

@bfabre01
Copy link
Author

bfabre01 commented Jun 5, 2019

@Morendil @sandcha : j'ai amélioré le fichier d'import des paramètres locaux.
Le test d'import des paramètres passe. Donc, c'est ok normalement.
Cet import peut surement être optimisé. Mais ça dépasse mon champ de compétence, et cette PR a déjà été assez (très) longue. Donc, à ce stade, sachant les revues déjà faites ci-dessus : soit vous trouvez l'import insuffisant, et je vous propose de prendre la main, soit on merge (sauf si bien sûr vous remarquez une erreur :-) ). Merci beaucoup !

@Morendil
Copy link
Contributor

Morendil commented Jun 7, 2019

C'est bon pour moi, @sandcha tu veux jeter encore un oeil ou je prends le merge ?


class exonere_th(Variable):
value_type = bool
default_value = False
Copy link
Contributor

@sandcha sandcha Jun 7, 2019

Choose a reason for hiding this comment

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

Etait à True précédemment. Qu'est-ce qui nous amène à la changer ? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

L'exonération est un dispositif ciblant un public particulier. Naturellement, je me suis donc dit qu'il était plus naturel de le faire à False par défaut.

Copy link
Member

Choose a reason for hiding this comment

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

Il était à True quand on ne voulait pas voir arriver une valuer de TH alors que l'on en la calculait pas. Donc le changement fait par @bfabre01 me paraît tout à fait judicieux désormais.

@sandcha
Copy link
Contributor

sandcha commented Jun 7, 2019

Encore merci @bfabre01 ! 👏
J'ai proposé les derniers éléments qui me manquaient sous forme de commits.
Il reste néanmoins la question du fichier stata/.do qui pour moi n'a sérieusement pas sa place dans le module (au sens Python). Si c'est bloquant pour vous de le déplacer dans scripts/, il doit a minima être exclu des assets à embarquer (cf. package exclude dans le setup.py).
@Morendil Je te passe la main pour la cloture 🙂

@sandcha
Copy link
Contributor

sandcha commented Jun 13, 2019

Fixup sur le MANIFEST.in appliqué pour propager l'exclusion du setup.py.
C'est bon pour moi.

Test du contenu de la wheel effectué via :

  • Build de la wheel avec python setup.py sdist bdist_wheel | grep assets
  • Revue de son contenu par : unzip -l OpenFisca_France-XX.0.0-py3-none-any.whl | grep assets (similaire au contenu du répertoire build/lib de sdist)

@sandcha
Copy link
Contributor

sandcha commented Jun 13, 2019

@bfabre01 Rebase en cours :)

@sandcha sandcha merged commit b2055b5 into master Jun 13, 2019
@sandcha sandcha deleted the taxe-habitation branch June 13, 2019 13:51
@bfabre01
Copy link
Author

@sandcha : Merci !

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.

6 participants