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

Various changes from IPP #858

Merged
merged 35 commits into from
Dec 13, 2017
Merged

Various changes from IPP #858

merged 35 commits into from
Dec 13, 2017

Conversation

fpagnoux
Copy link
Member

@fpagnoux fpagnoux commented Dec 5, 2017

Breaking changes:

  • asi_aspa_condition_nationalite vaut maintenant True par défaut
  • rsa_condition_nationalite vaut maintenant True par défaut
  • retraite_imposable est maintenant une variable mensuelle

@@ -254,6 +251,85 @@ def formula(foyer_fiscal, period):
return csg_cap_lib + prelsoc_cap_lib + crds_cap_lib


class impots_directs_menage(Variable):
Copy link
Member Author

Choose a reason for hiding this comment

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

@benjello

Toutes les variables qui suivent, ajoutées dans taxipp, ne comportent pas de description.
Par ailleurs, impots_directs_menage est un nom étrange, vu que impots_directs est déjà une variable qui concerne les ménages.

Est-ce que ces variables ont vocation à être dans le modèle commun de la législation ? Si oui, est-ce que tu peux me donner pour chacune d'entre elles une description et un nom de variable plus explicite ?

Merci !

Copy link

Choose a reason for hiding this comment

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

@fpagnoux : on utilise les variables terminant par _menage pour sortir des stats sur la décomposition des niveaux de vie par décile (le niveau de vie étant par définition au niveau du ménage). La variable revenu_disponible_famille est aussi pour nos besoins spécifiques. Donc, toutes ces variables n'ont pas vocation à entrer dans OF France. Si tu peux les ajouter dans la réforme taxipp, comme tu as fait pour le reste, ça serait top. Pour la signification, les variables finissant par _menage sont des montant au niveau du ménage ramené au nombre d'unité de consommation (du coup, la différence entre impots_directs et impots_directs_menage, c'est la division du nombre d'UC). Ce sont de très mauvais noms en effet... Ne T'occupe pas des docstrings, on le fera. Car je pense qu'il faut surtout changer le nom de ces variables, ce qui implique de faire ces changements là où elles sont utilisées.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, super, je déplace tout ça sur la réforme taxipp !

@fpagnoux fpagnoux force-pushed the ipp-changes branch 4 times, most recently from aaee498 to fd83883 Compare December 8, 2017 00:11
Copy link
Contributor

@Anna-Livia Anna-Livia left a comment

Choose a reason for hiding this comment

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

Review in progress :)

CHANGELOG.md Outdated
* Périodes concernées : toutes.
* Zones impactées : `/mesures`.
* Détails :
- Corrige le calcul de `uc` (Unités de consommation d'un ménage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourrais t'on en profiter pour changer le terme uc en unite_consommation_menage ?

Copy link
Member

Choose a reason for hiding this comment

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

Disons que les unités de consommation sont un concept statistique au niveau du ménage et ce serait redondant et l'entité précise également la chose. L'abbréviation est très courante dans le milieu de la statistique publique mais on peut mettre unites_de_consommation même si c 'est un peu long.


<!-- -->

* Évolution du système socio-fiscal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Est ce que toutes les zone impactées et détails pourraient être mis ensemble : éviter de répéter "+* Évolution du système socio-fiscal./* Périodes concernées : toutes." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Si tu as une idée de comment le faire, en s'assurant qu'il soit possible d'associer facilement à chaque zone adaptée ses détails, je suis preneur.

Je pense qu'il est important de ne pas perdre cette information pour les utilisateurs, car cette PR n'a pas de cohérence fonctionelle, donc un user doit pouvoir lire les changements séparément pour identifier facilement ceux qui l'impactent.

@@ -13,23 +13,12 @@ class uc(Variable):
label = u"Unités de consommation"
definition_period = YEAR

def formula(self, simulation, period):
'''
Calcule le nombre d'unités de consommation du ménage avec l'échelle de l'INSEE
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce que cette explication mériterait de se retrouver dans le label de la variable ?

Copy link
Member

Choose a reason for hiding this comment

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

Oui. Unités de consommation selon l'échelle de l'INSEE

@@ -68,21 +57,21 @@ class revenu_disponible(Variable):

def formula(self, simulation, period):
revenus_du_travail_holder = simulation.compute('revenus_du_travail', period)
pen_holder = simulation.compute('pensions', period)
rev_cap_holder = simulation.compute('revenus_du_capital', period)
pensions_holder = simulation.compute('pensions', period)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

pen_holder = simulation.compute('pensions', period)
rev_cap_holder = simulation.compute('revenus_du_capital', period)
pensions_holder = simulation.compute('pensions', period)
revenus_du_capital_holder = simulation.compute('revenus_du_capital', period)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌


def formula(individu, period, parameters):
salaire_net = individu('salaire_net', period.start.period('month', 6).offset(-6), options = [ADD])
# D'après service-public.fr, le salaire n'est pas évalué de la même manière suivant si l'enfant est étudiant ou salarié/apprenti/stagiaire.
Copy link
Contributor

Choose a reason for hiding this comment

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

En lisant service-public.fr, j'ai l'impression que ce n'est pas le salaire qui est évalué différemment, c'est les condition pour le dépassement : dès le dépassement, pour les stagiaire et apprentis, si la moyenne sur les 6 derniers mois dépasse le plafond mensuel pour les scolarisé.
Par contre, le plafond semble être toujours 55% du SMIC.

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 je précise le commentaire.

@@ -756,13 +752,14 @@ class af_nbenf_fonc(Variable):
# Hack pour éviter une boucle infinie
Copy link
Contributor

Choose a reason for hiding this comment

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

c'est quoi le hack ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Voir plus bas

# Note : Cette variable est "instantanée" : quelque soit la période demandée, elle retourne la valeur au premier
# jour, sans changer la période.
salaire_de_base = simulation.calculate_add('salaire_de_base', period.start.period('month', 6).offset(-6))
salaire_de_base_mensualise = simulation.calculate_add('salaire_de_base', period.start.period('month', 6).offset(-6)) / 6
law = simulation.parameters_at(period.start)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pplutot que law?

Copy link
Member

Choose a reason for hiding this comment

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

Cette remarque dépasse cette occurrence, il nous faut comment doit-on écrire les appels aux paramètres législatifs et corriger partout.

law = simulation.parameters_at(period.start)
nbh_travaillees = 169
smic_mensuel_brut = law.cotsoc.gen.smic_h_b * nbh_travaillees
autonomie_financiere = (salaire_de_base / 6) >= (law.prestations.prestations_familiales.af.seuil_rev_taux * smic_mensuel_brut)
autonomie_financiere = (
Copy link
Contributor

Choose a reason for hiding this comment

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

pourquoi ne pas utiliser la variable autonomie_financière ?

Copy link
Member Author

@fpagnoux fpagnoux Dec 11, 2017

Choose a reason for hiding this comment

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

Je crois que c'est justement le fameux hack pour éviter une boucle infinie.

On a besoin du nombre d'enfants au sens de la CAF pour déterminer le supp_familial_traitement du fonctionnaire, qui rentre en compte dans le salaire_imposable, et donc dans le salaire_net. Or autonomie_financiere dépends du salaire_net. Appeler autonomie_financiere ici créerait donc une boucle infinie.

Dans la monde réel, ça ne pose pas de problème car le salaire net du parent dépends du salaire net de l'enfant, mais en vectoriel tout est à plat, on calcule toujours les variables pour tous les individus en même temps.

Je rajoute un commentaire pour expliquer 👍

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense que ce serait intéressant d'avoir une courte description de la réforme.

Copy link
Member

Choose a reason for hiding this comment

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

On peut virer cette réforme et ma remonter vers les outils IPP dans un premier temps.
On est prêt à tout partager mais s'il n'y a pas d'usage et que cela bloque, je préfère que l'on la déporte en aval.

rsa_base_ressources = famille('rsa_base_ressources', period)
parameters_rsa = parameters(period).prestations.minima_sociaux.rsa
parameters_prestation_unifiee = parameters(period).prestation_unifiee
# loyer_imputes = famille('loyer_imputes')
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi est-ce que cette ligne est commentée ?

Copy link
Member

@benjello benjello Dec 12, 2017

Choose a reason for hiding this comment

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

Les loyers imputés sont les loyers fictifs.
Encore une fois déportons cette réforme en aval.

categorie_salarie = simulation.calculate('categorie_salarie', period = this_year)
contrat_de_travail = simulation.calculate('contrat_de_travail', period = this_year)
heures_remunerees_volume = simulation.calculate('heures_remunerees_volume', period = this_year)
hsup = simulation.calculate('hsup', period = this_year)
Copy link
Contributor

Choose a reason for hiding this comment

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

Peut-on rendre la variable plus explicite ? heures_supplementaires ? heures_sup ?

Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi pas. Je suggère heures_supplementaires

taux_abattement = csg_deductible.abattement.rates[0]
try:
seuil_abattement = csg_deductible.abattement.thresholds[1]
except IndexError: # Pour gérer le fait que l'abattement n'a pas toujours était limité à 4 PSS
Copy link
Contributor

Choose a reason for hiding this comment

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

Qu'est ce que PSS ?

Copy link
Member

Choose a reason for hiding this comment

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

plafond de la sécurité sociale

del baremes_collection[name]

for categorie in ['prive_non_cadre', 'prive_cadre']:
test = set(
Copy link
Contributor

Choose a reason for hiding this comment

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

Est ce que c'est normal que cette variable s'appelle test ?

name for name, bareme in salarie[categorie].iteritems()
if isinstance(bareme, MarginalRateTaxScale)
)
assert target[categorie] == test, 'target: {} \n test {}'.format(target[categorie], test)
Copy link
Contributor

Choose a reason for hiding this comment

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

Je ne comprends pas très bien ce que test cet assert. Est ce qu'on pourrait rendre le message d'erreur plus explicite ?

Copy link
Member

Choose a reason for hiding this comment

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

Mettre aussi cela en aval

@@ -100,7 +100,7 @@ def check_1_parent_2_enfants_1_column(column_name, year):


def test_1_parent_2_enfants_1_column():
for column_name, column in tax_benefit_system.column_by_name.iteritems():
for column_name, column in tax_benefit_system.variables.iteritems():
Copy link
Contributor

@Anna-Livia Anna-Livia Dec 12, 2017

Choose a reason for hiding this comment

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

🙌 :


import datetime

#%%
Copy link
Contributor

Choose a reason for hiding this comment

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

à quoi sert cette ligne ?


from __future__ import division


Copy link
Contributor

Choose a reason for hiding this comment

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

Serait-il possible d'inclure quelques lignes de contexte au début des fichiers qui décrivent une réforme ?

@fpagnoux
Copy link
Member Author

I removed the changes in reforms that raised a lot of (justified) questions, as they were not ready to be merged 🙂.

Is it good to merge now ?

@fpagnoux fpagnoux requested a review from Anna-Livia December 12, 2017 20:33
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