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

Complete units #1022

Merged
merged 12 commits into from
Aug 10, 2018
Merged

Complete units #1022

merged 12 commits into from
Aug 10, 2018

Conversation

benjello
Copy link
Member

@benjello benjello commented Jun 28, 2018

Merci de contribuer à OpenFisca ! Effacez cette ligne ainsi que, pour chaque ligne ci-dessous, les cas ne correspondant pas à votre contribution :)

  • Amélioration technique.
  • Périodes concernées : toutes.
  • Zones impactées : plein de paramètres législatifs.
  • Détails :
    • Renseigne les unités manquantes

Ces changements (effacez les lignes ne correspondant pas à votre cas) :

  • Modifient des éléments non fonctionnels ou secondaires de ce dépôt.

Quelques conseils à prendre en compte :

@benjello benjello changed the title Complete units WIP: Complete units Jun 28, 2018
@benjello
Copy link
Member Author

benjello commented Jun 28, 2018

Pour ceux qui désirent contribuer:

  • il faut utiliser les améliorations fourni par cette branche de core afin de pouvoir faire tourner le script openfisca_france/scripts/parameters/explore_parameters_unit.py qui détecte les noeuds sans unités
  • n'oubliez pas de pousser rapidement vos changements ou d'annoncer quelle partie vous traitez afin de ne pas dupliquer le travail déjà pénible

@benjello
Copy link
Member Author

benjello commented Jun 29, 2018

Je commence une liste des unités manquantes pour la France:

  • mois, jours, heures -> month, day, hour
  • date (année) -> date
  • nombre de parts fiscales -> custom pour France
  • nombre d'individus/enfants
  • unités monétaires qui évolue: smic/smic horaire/plafond de la sécurité sociale

@benjello
Copy link
Member Author

@guillett @fpagnoux @sancha @Anna-Livia : au fur et à mesure que j'attribue des unités des cas moins simples apparaissent. Votre avis m'intéresse !

@maukoquiroga @MattiSG : il ya clairement des unités génériques indépendantes du pays mais il y aura sûrement des unités propre à chaque pays (smic, smic horaire etc). Votre avis m'intéresse aussi !

@fpagnoux
Copy link
Member

fpagnoux commented Jul 6, 2018

Tout ça pose des questions intéressantes sur le role de unit.

Pour le moment, c'est seulement une métadonnée qui n'est pas techniquement utilisée par core (seulement exposée depuis openfisca/openfisca-core#681). Si on n'en reste là, il n'y a pas de problème à ce que différents pays aient des unités propre, vu que c'est un champs libre. Mais il reste pertinent de se mettre d'accord sur des conventions pour que des outils comme le legislation explorer puisse par exemple avoir un formattage pertinent en fonction de l'unité.

Il faut par exemple qu'on se mette d'accord sur les paramètres exprimées en unité monétaire absolue. Possibilités:

  • currency (n'indique pas la monnaie utilisée)
  • EUR (simple, mais peut rendre difficile l'identification des paramètres monétaires dans le cas général)
  • currency/EUR (plus complexe, mais contient toute l'information)

unités monétaires qui évolue: smic/smic horaire/plafond de la sécurité sociale

Je me demande jusqu'où on veut aller avec ça. Est-ce que c'est simplement avoir une interface qui peut afficher "en % du smic horaire brut", où est-ce qu'on voudrait lier un paramètre à un autre?

@benjello
Copy link
Member Author

benjello commented Jul 6, 2018

@fpagnoux : il y a un besoin qui est commun à tous les country package de pouvoir repérer les paramètres qui sont données en monnaie nominale, par exemple pour inflater une législation (exemple) donc la présence de currency s'impose.
Doit-on donner également le code iso de la monnaie du pays dans unit ? Je ne suis pas contre car il peut y avoir des usages légitimes (legislation-explorer etc).

@benjello
Copy link
Member Author

benjello commented Jul 6, 2018

En ce qui concerne les paramètres qui sont des multiples des autres, je pense qu'ils serait utile de pouvoir faire le lien. Peut être un truc du genre:

unit: /cotisations_sociales.plafond_securite_sociale

pourrait être utile.

Il pourrait être utile de réfléchir à la façon dont on pourrait rendre l'unité en texte pour certains affichages, c.-à-d. dans notre cas "en nombre de plafond de la sécurité sociale" ou "en pourcentage du plafond de la sécurité sociale".

Mais le plus urgent selon moi est le premier point pour deux raisons:

  • repérer les paramètres qui de fait changent une année donnée via le changement d'un autre paramètre
  • lier, lors de la présentation d'un paramètre, ce dernier à sa véritable unité

@MattiSG
Copy link
Member

MattiSG commented Jul 12, 2018

Comme expliqué dans openfisca/openfisca-core#681 (comment), je suis en faveur de permettre l'innovation et l'exploration à l'échelle de la France sans chercher à normer précisément l'usage pour le moment.

Je suis par exemple dubitatif sur la référence à d'autres paramètres / variables en raison du coût de maintenance et de la complexité, mais il s'agit d'une évaluation personnelle et vue sous un angle d'ingénieur. Peut-être la plus-value sera réellement grande et l'usage important. Inversement, si on réalise à l'usage qu'il y a seulement une dizaine de variables concernées, le coût n'est probablement pas justifié.

En ce qui concerne le format, la forme qui passe le plus à l'échelle et ouvre le plus de possibilités est l'usage d'URI.
Concrètement, cela signifierait a minima d'utiliser un schéma d'URI arbitraire tel que currency://EUR, avec pour objectif de permettre l'utilisation de données liées par l'usage de référentiels globaux, comme par exemple https://www.wikidata.org/wiki/Q4916.

Cette vision est néanmoins toujours difficile à activer car elle semble inutilement complexe tant qu'on n'a pas débloqué des usages réels. Je crois donc qu'une première passe avec currency<délimiteur><CODE> permettra déjà d'illustrer l'utilité, et qu'un rechercher-remplacer global sera toujours possible ultérieurement.

Le choix entre : et / devrait selon moi se faire par validation de la non-ambiguité de : en YAML, que ce soit pour les parsers, les outils de coloration syntaxique, et les utilisateurs.

@fpagnoux
Copy link
Member

fpagnoux commented Jul 12, 2018

Le choix entre : et / devrait selon moi se faire par validation de la non-ambiguité de : en YAML, que ce soit pour les parsers, les outils de coloration syntaxique, et les utilisateurs.

Pour ce qui est des parsers et les outils de coloration, l'espace après le : est suffisant pour faire la différence entre un object est une string qui contient un :. Pas sûr que ce soit limpide pour les utilisateurs par contre.

image

/ a par contre pour moi un gros inconvénient:

The downside of using / though is that it can be confused with a division. We would have for instance EUR/m2 and currency/EUR where / has a totally different meaning.

Peut -être qu'on devrait se rabattre sur currency-EUR 🤔 ?

@MattiSG
Copy link
Member

MattiSG commented Jul 12, 2018 via email

@benjello
Copy link
Member Author

benjello commented Aug 8, 2018

Je soumets cette PR telle quelle, c'est à dire sans préciser 'currency-EUR`ou 'currency-FRF'
Dès qu'elle est acceptée, j'ouvre un ticket pour régler ce problème qui n'est pas très urgent.

@benjello benjello changed the title WIP: Complete units Complete units Aug 9, 2018
@benjello
Copy link
Member Author

benjello commented Aug 9, 2018

@fpagnoux @Anna-Livia : j'aimerait bien faire passer cette PR inoffensive assez rapidement pour transfert au CASD etc

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 n'ai pas vérifié toutes les unités, mais ça l'air bon.

@@ -12,6 +12,7 @@ class ass_precondition_remplie(Variable):
entity = Individu
label = u"Éligible à l'ASS"
definition_period = MONTH
set_input = set_input_dispatch_by_period
Copy link
Member

Choose a reason for hiding this comment

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

Ça a un rapport avec le reste de la PR 😄 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Non c'est opportuniste comme le commit l'indique ;-)

@@ -1,3 +1,5 @@
description: Loyer minimun lo
Copy link
Member

Choose a reason for hiding this comment

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

Je crois que @guillett essaye de rendre plus "déterministe" le contenu des fichiers YAML, et que ça implique de garder les clés dans l'ordre alphabétique.

Copy link
Member Author

Choose a reason for hiding this comment

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

@guillet: est-ce grave ou tu peux refaire un coup de moulinette ex-post (ou me dire comment le faire) ?

@@ -1,5 +0,0 @@
description: Fixe le Rmi dans les simulations
Copy link
Member

Choose a reason for hiding this comment

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

Nettoyage appréciable, mais techniquement c'est non-rétro compatible, donc peut être pas à glisser dans cette PR

Copy link
Member Author

Choose a reason for hiding this comment

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

@fpagnoux : tu me conseilles de faire une PR à part pour ce point là ?

@@ -1,5 +0,0 @@
description: Fixe le Rmi dans les simulations
Copy link
Member

Choose a reason for hiding this comment

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

Nettoyage appréciable, mais techniquement c'est non-rétro compatible, donc peut être pas à glisser dans cette PR

@@ -1,22 +1,25 @@
reference: ipp
un_enfant:
Copy link
Member

Choose a reason for hiding this comment

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

Comme plus haut, même si ça se lit mieux, je crois que ça va à l'encontre de ce qu'essaye de faire @guillett

@@ -0,0 +1,53 @@
# -*- coding: utf-8 -*-

Copy link
Member

Choose a reason for hiding this comment

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

Est-ce qu'on peut ajouter une courte description de ce que fait ce script?

@benjello
Copy link
Member Author

@fpagnoux is this ok now ? (Should I rebase before merging)

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.

Yep, should be rebased

@fpagnoux
Copy link
Member

Rebased and ready to merge when tests pass`

@fpagnoux fpagnoux merged commit ffbae9e into master Aug 10, 2018
@fpagnoux fpagnoux deleted the complete-units branch August 10, 2018 16:00
@benjello
Copy link
Member Author

Thanks @fpagnoux !

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.

4 participants