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

Mise à jour du taux de versement transport #788

Closed
wants to merge 6 commits into from
Closed

Mise à jour du taux de versement transport #788

wants to merge 6 commits into from

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Jul 2, 2017

  • Évolution du système socio-fiscal.
  • Périodes concernées : à partir du 01/07/2017
  • Zones impactées : openfisca_france/assets/versement_transport.
  • Détails :

Ces changements

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

@Morendil Morendil requested a review from fpagnoux July 7, 2017 06:24
@fpagnoux
Copy link
Member

Merci @Morendil pour cette mise à jour !

Pour info, tu as les droits d'écriture sur ce dépôt, donc n'hésite pas à créer tes feature branches ici, c'est plus simple 🙂 .

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.

Je ne suis pas en mesure de valider manuellement toutes les modifications apportées par cette PR.

Est-ce que le JSON a été édité à la main ? Si oui, merci pour cette tâche fastidieuse (et mergeons). Si non, peut-on documenter le processus de mise à jour ?

@MattiSG
Copy link
Member

MattiSG commented Jul 24, 2017

La mise à jour a apparemment été effectuée depuis une source de données de la CAF.

@fpagnoux où voudrais-tu enregistrer cette référence ? Il me semble que ce taux devrait être enregistré au format standard des paramètres et stocker la référence de la même manière.

@laem
Copy link
Contributor

laem commented Jul 26, 2017

@fpagnoux c'est un script qui les génère (heureusement :-) )

On va mettre ce script documenté sur un dépôt.

@MattiSG Oui l'URSSAF. Etant donné la taille du fichier de taux, je pense que ça posera problème de les stocker en XML comme les paramètres à la fois en termes de poids dans le repo, de temps de parsing ou d'accès (je peux me tromper mais je crois que l'accès aux paramètres dans une formule est coûteux).
Voir cette PR où ma tentative de les stocker en YAML ralentissait l'initialisation d'OpenFisca.

@laem
Copy link
Contributor

laem commented Jul 26, 2017

@Morendil je pense qu'il faut introduire quelques tests pour la nouvelle période, notamment au vu de la nature massive de la modification, dont le diff est illisible. Voir la dernière PR sur le sujet.
Je m'en charge !

@laem
Copy link
Contributor

laem commented Jul 26, 2017

Voilà ce que je te propose (à faire tourner, je ne l'ai pas fait !), à ajouter en fin de tests/formulas/taux_versement_transport.yaml

# > juillet 2017
# https://www.urssaf.fr/portail/home/actualites/toute-lactualite-employeur/taux-versement-transport--nouvel.html


- period: "month:2017-07"
  absolute_error_margin: 0.00001
  input_variables:
    depcom_entreprise: "65226" # Commune IBOS
    effectif_entreprise: 11
  output_variables:
    taux_versement_transport: 0.0105


- period: "month:2017-07"
  absolute_error_margin: 0.00001
  input_variables:
    depcom_entreprise: "71258" # Commune LEYNES
    effectif_entreprise: 11
  output_variables:
    taux_versement_transport: 0.008

# Commune non présente (taux nul) précédemment
- period: "month:2017-07"
  absolute_error_margin: 0.00001
  input_variables:
    depcom_entreprise: "51614" #Commune VERZY
    effectif_entreprise: 11
  output_variables:
    taux_versement_transport: 0.0015

@fpagnoux
Copy link
Member

@fpagnoux où voudrais-tu enregistrer cette référence ? Il me semble que ce taux devrait être enregistré au format standard des paramètres et stocker la référence de la même manière.

Faute de pouvoir les intégrer dans le code, a minima dans le changelog.

CHANGELOG.md Outdated
@@ -1,5 +1,13 @@
# Changelog

### 18.5.5 - [#788](https://github.com/openfisca/openfisca-france/pull/788)
Copy link
Contributor

Choose a reason for hiding this comment

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

La version doit être changée et le code rebasé :)

Copy link
Member

Choose a reason for hiding this comment

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

@Anna-Livia dans le cas de contributeurs externes, n'hésitons pas à prendre à notre charge ces contraintes administratives. On peut expliquer et pousser à monter en compétence, mais pour des premières contributions, ne décourageons pas avec ce genre de contraintes.

@Morendil Pour moi le seul élément véritablement bloquant est cette fameuse description de la source des données, que je préférais te laisser formuler. Je peux prendre si tu préfères, mais je ne garantis pas de respecter toutes les contraintes qui avaient l'air d'exister 😉

@MattiSG
Copy link
Member

MattiSG commented Aug 18, 2017

Ne pourrait-on pas simplement utiliser l'attribut reference pour référencer l'API CAF, ou le dépôt qui en assemble les données ?

@Morendil
Copy link
Contributor Author

@MattiSG je ne sais pas quel attribut reference je pourrais utiliser, c'était le sens de ma question à @fpagnoux - pour l'instant c'est dans le Changelog, je rebase et pousse

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Ok, compris. En réalité versement_transport.taux n'est pas chargé comme un paramètre 😞
Il est utilisé comme une global 😱

Je ne vois donc pas de meilleur endroit où mettre la référence ailleurs que dans le JSON lui-même et le README, sauf qu'un fichier JSON ne peut pas avoir de commentaire… je ne vois donc pas mieux que le README aujourd'hui.

@MattiSG
Copy link
Member

MattiSG commented Aug 18, 2017

Superseded by #806.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants