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

Fiabilisation des calculs d'impôts #1193

Merged
merged 19 commits into from
Oct 31, 2018
Merged

Fiabilisation des calculs d'impôts #1193

merged 19 commits into from
Oct 31, 2018

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Oct 26, 2018

  • Évolution du système socio-fiscal.
  • Périodes concernées : toutes.
  • Zones impactées : impot_revenu.
  • Détails :
    • Nouveaux cas de tests faisant apparaitre des divergences entre OpenFisca et le calculateur DGFIP.
    • Correctifs pour résoudre ces divergences.

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 ! :)

@Morendil
Copy link
Contributor Author

@claireleroy Yeah ça passe pour reduc_locmeu :)

@Morendil
Copy link
Contributor Author

@claireleroy Si tu veux on peut extraire une PR pour juste ce travail-là et continuer sur autre branche pour les autres tests ? Ou bien peut-être que ça vaut le coup d'embarquer aussi locmeu plafonnée, qu'en dis-tu ?

@claireleroy
Copy link
Contributor

@Morendil Oui comme tu veux ! je vais surement continuer à regarder ces tests demain mais pas sur que j'arrive à les régler tous demain.

@claireleroy claireleroy force-pushed the fiabilisation-impots branch 3 times, most recently from fd0658a to c70ee5b Compare October 30, 2018 15:27
@claireleroy
Copy link
Contributor

@Morendil Les tests passent tous maintenant chez moi, sauf ceux pour 2014 qui font une erreur que je n'arrive pas à comprendre. Peut-être as tu une idée ?

======================================================================
ERROR: unittest.case.FunctionTestCase (check)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\github\openfisca-core\openfisca_core\tools\test_runner.py", line 152, in check
    _run_test(period_str, test, verbose, only_variables, ignore_variables, options)
  File "c:\github\openfisca-core\openfisca_core\tools\test_runner.py", line 245, in _run_test
    simulation.calculate(variable_name, requested_period),
  File "c:\github\openfisca-core\openfisca_core\simulations.py", line 171, in calculate
    array = self._run_formula(variable, entity, period, extra_params, max_nb_cycles)
  File "c:\github\openfisca-core\openfisca_core\simulations.py", line 265, in _run_formula
    array = formula(entity, period, parameters_at, *extra_params)
  File "c:\github\openfisca-france\openfisca_france\model\prelevements_obligatoires\impot_revenu\ir.py", line 3265, in formula
    rsa_act_i = foyer_fiscal.members('rsa_activite_individu', period, options = [ADD])
[ ... ]
  File "c:\github\openfisca-france\openfisca_france\model\prestations\prestations_familiales\paje.py", line 152, in formula_2004
    date_plus_jeune = famille.reduce(famille.members('date_naissance', period), maximum, datetime64('1066-01-01'))
  File "c:\github\openfisca-core\openfisca_core\entities.py", line 567, in reduce
    result = reducer(result, self.value_nth_person(p, filtered_array, default = neutral_element))
TypeError: invalid type promotion
-------------------- >> begin captured logging << --------------------
openfisca_core.tools.test_runner: ERROR: regime_benef_reel_BIC_pro_sans_cga.yaml: regime_benef_reel_BIC_pro_sans_cga - 2014
--------------------- >> end captured logging << ---------------------

@Morendil
Copy link
Contributor Author

Morendil commented Oct 30, 2018

@claireleroy Pas vraiment, mais je vois que tout fonctionne sur l'intégration continue; peut-être as-tu une ancienne version de Core ? Peux-tu faire un pip list pour voir quelle version de Core tu as localement ? J'essaierai de reproduire si ce n'est pas la dernière version.

@claireleroy
Copy link
Contributor

@Morendil J'étais sur les versions suivantes, quand je fais pip list :

  • 23.1.2 de Core
  • 1.4.0 de Country-Template
  • 18.11.0 de France

Mais effectivement, je vois que les tests passent ici donc c'est surement juste un problème chez moi.

@claireleroy
Copy link
Contributor

@Morendil Pour la suite, je reprends la piste avancée dans #1024, où tu avais proposé d'extraire 3 PR :

  • une nouvelle avec les test générés non passants, qu'on corrigerait rapidement
  • une nouvelle avec seulement les tests générés et passants (qu'on peut merger mais voir ci-dessous la question des performances)
  • on garderait les scripts de génération dans une PR non mergée, le temps de prioriser la migration du code à Python 3.

J'ai donc l'impression qu'on a fait la première PR (#1193) et qu'on pourrait rebaser et merger ? On peut donc passer à la seconde PR je pense ?

Si je comprends bien, l'inquiétude concerne le nombre de tests importants (262) que cela représente. C'est tout à fait justifié. Cependant, étant donné que le code de l'impôt est une partie jamais testée d'OpenFisca (en comparaison de la partie prestations sociale par exemple), je trouverais ça vraiment bien qu'on en sélectionne au moins une partie, pour faire partie de la batterie de tests lancés à chaque PR. Aujourd'hui quand quelqu'un fait une modification dans OpenFisca, rien ne garantit qu'il ne casse pas le calcul de l'impôt (ce qui m'attristerais beaucoup vu le temps passé par moi et par d'autres pour essayer d'améliorer et fiabiliser ce calcul justement).

@Morendil
Copy link
Contributor Author

@claireleroy Oui je suis bien d'accord. Il est possible aussi que l'impact en performance de ces tests soit moindre que ce que je craignais, parce que les tests sur l'impôt ont peut-être une profondeur moindre.

Je te propose:

  • de merger celle-ci (je prends le rebase et mise à jour du Changelog)
  • d'ouvrir la PR avec les 200 et quelques autres, mesurer le temps supplémentaire à l'exécution, et en discuter avec l'équipe en fonction de ce résultat

@Morendil
Copy link
Contributor Author

@claireleroy Tu peux mettre un "approve" pour pouvoir merger ?

@Morendil Morendil force-pushed the fiabilisation-impots branch from 833bb76 to a9faebb Compare October 31, 2018 12:37
@Morendil Morendil force-pushed the fiabilisation-impots branch from a9faebb to 794af7e Compare October 31, 2018 12:38
@Morendil Morendil merged commit c0c1fe3 into master Oct 31, 2018
@Morendil Morendil deleted the fiabilisation-impots branch October 31, 2018 14:43
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.

2 participants