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

Corrige le calcul de l'ASS #1172

Merged
merged 18 commits into from
Oct 22, 2018
Merged

Corrige le calcul de l'ASS #1172

merged 18 commits into from
Oct 22, 2018

Conversation

aguillouzouic
Copy link
Contributor

@aguillouzouic aguillouzouic commented Oct 17, 2018

Fix Issue #1099

  • Changement majeur.
  • Zones impactées :
    • model\prestations\minima_sociaux\ass.py.
    • tests\formulas\ass.yaml
    • prestations/minima_sociaux/rsa
  • Détails :
    • Passe à une définition de l'ASS au niveau "Individu" au lieu de "Famille"
    • Ajoute les revenus du capital au calcul de la base revenu de l'individu demandeur de l'ASS et de son conjoint éventuel
    • Ajoute un test d'un ménage avec individus tous deux éligibles à l'ASS.

NB: Problème sur le dernier point car le test accepte à la fois un vecteur avec chaque ASS et un scalaire avec le montant de l'ASS seulement pour l'individu de référence.


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

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

Quelques conseils à prendre en compte :

Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

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

Voir commentaires et potentiel bug dans le processus de test

chomage_net: 0
aah: 0
output_variables:
ass: 494.40
Copy link
Member

Choose a reason for hiding this comment

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

Je tenais à signaler un potentiel bug qu'a découvert @aguillouzouic.
L'ASS est ici calculée au niveau individuel. les deux personnes touchent le même montant.
Or on ne renseigne qu'une valeur pour une valeur individuelle alors que le vecteur devrait être de taille 2.
On n'a pas d'erreur sur la taille du vecteur. Et le test est validé. Ce qui est problématique.

Il faut voir la cause de ce problème puis corriger ce test
par

output_variables:
    ass: [494.40, 494.40] 

cc @Morendil @sandcha @guillett

Copy link
Member

Choose a reason for hiding this comment

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

C'est peut-être de la magie numpy.

Copy link
Member

Choose a reason for hiding this comment

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

Sûrement @guillett .
Mais @Morendil @sandcha @fpagnoux ou @maukoquiroga, cela devrait vous intéresser de régler ce faux positif au niveau de core. Ce test ne devrait pas passer mais il passe.
On va corriger pour conserver une forme convenable mais je ne vosi pas comment signaler le problème sur core proprement donc j'aimerais bien que quelqu'un s'en charge et qu'on puisse merger cette PR tranquilement.

Merci !

Copy link
Member

Choose a reason for hiding this comment

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

Yep, numpy magics:

(np.asarray([200]) == np.asarray([200, 200])).all()
>>> True

Copy link
Member

Choose a reason for hiding this comment

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

@aguillouzouic. peux-tu corriger le test stp.

@fpagnoux @maukoquiroga @sandcha : je vous laisse gérer la suite à donner où j'ouvre une issue dans core pointant ici.

Copy link
Member

@fpagnoux fpagnoux Oct 19, 2018

Choose a reason for hiding this comment

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

Il y a des réflexions en ce moment sur le format des tests YAML. Je traîne sur ce sujet, mais je vais essayer de formaliser mes reflexions et de faire des propositions en début de semaine prochaine.

Mais je me demande si ce "potentiel bug" est vraiment un problème. Dans quel cas concret est-ce qu'un test donnerait un résultat inattendu et fonctionnellement faux? Si la deuxième personne avait un ASF de 200 (alors que le test indique 494.4) il y aurait bien une erreur.

Le comportement est juste que si un scalaire est fourni comme valeur attendue, et que la valeur réelle est vectorielle, le test runner conclut que le test est positif si tous les éléments du vecteur sont égaux au scalaire. Ça ne me semble pas déraisonnable.

@benjello benjello requested a review from guillett October 17, 2018 16:50
CHANGELOG.md Outdated
@@ -1,5 +1,16 @@
# Changelog

### 27.0.3 [#1172](https://github.com/openfisca/openfisca-france/pull/1172)
Copy link
Member

Choose a reason for hiding this comment

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

Le déplacement de l'ASS de la famille à l'individu est un changement majeur.

Copy link
Member

Choose a reason for hiding this comment

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

Il faut passer à 28.0.0

@@ -18,17 +18,16 @@ class ass_precondition_remplie(Variable):
class ass(Variable):
value_type = float
label = u"Montant de l'ASS pour une famille"
Copy link
Member

Choose a reason for hiding this comment

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

Le label n'est plus correct.

Copy link
Member

@guillett guillett left a comment

Choose a reason for hiding this comment

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

J'ai fait une première passe, c'est vraiment cool vos contributions !

@aguillouzouic aguillouzouic changed the title [WIP] Ajoute les revenus du capital au calcul de l'ASS Ajoute les revenus du capital au calcul de l'ASS Oct 18, 2018
@aguillouzouic aguillouzouic changed the title Ajoute les revenus du capital au calcul de l'ASS Corrige le calcul de l'ASS Oct 18, 2018
guillett
guillett previously approved these changes Oct 18, 2018
Copy link
Member

@guillett guillett left a comment

Choose a reason for hiding this comment

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

LGTM

@guillett guillett dismissed their stale review October 18, 2018 14:28

Oups, tests need to be updated

@aguillouzouic
Copy link
Contributor Author

@guillett
Les tests mes-aides cassent à cause de la définition de l'ASS au niveau individu, mais ils sont très nombreux : est ce qu'il y a par hasard une routine pour tout modifier d'un coup?
Ou les supprimer pour la plupart vu qu'ils définissent généralement (du moins tous ceux que j'ai vus) une ASS à 0 qui doit être la valeur par défaut de l'aide.

Merci !

@guillett
Copy link
Member

@aguillouzouic je vais faire une première passe de suppression massive (j'ai fait ça sur l'ASI la semaine dernière).

Je vais commiter directement sur la branche. Pourras-tu faire

  • git fetch --all pour récupérer la dernière version de la branche
  • git rebase origin/ass_capital et ajouter tes nouveaux développements en local au dessus des modifications que j'aurais apportées.

@aguillouzouic
Copy link
Contributor Author

@guillett
J'ai l'impression qu'il reste des problèmes notamment sur les ludwigtest et dans le test 59415de47770d2503676954c.

Est ce que tu souhaites que je corrige ça?

@guillett
Copy link
Member

@aguillouzouic oui je veux bien.
@Morendil c'est dommage qu'un seul test soit mis en avant en cas d'erreur de format.

@Morendil
Copy link
Contributor

@guillett Oui on a d'ailleurs ce problème aussi sur les tests fonctionnels :) ça me tient à coeur d'améliorer ça aussi

@aguillouzouic
Copy link
Contributor Author

À noter : le test 544123db288947997ea40dc2 avait été modifié dans le nettoyage et cassait sur le check lint, je l'ai corrigé pour qu'il passe mais le test en lui-même me parait quand même étrange, si tu souhaites regarder @guillett.

@benjello benjello dismissed their stale review October 22, 2018 08:25

Can(t approve them !

@aguillouzouic aguillouzouic merged commit 04e51e0 into master Oct 22, 2018
@Morendil
Copy link
Contributor

@aguillouzouic La norme sur France est d'éviter les merge commits, on privilégie le rebase…

@benjello
Copy link
Member

@Morendil, à la décharge de @aguillouzouic, je ne crois pas qu'il sache ce qu'est un rebase. Mais on lui ontre cela demain promis ;-)

@aguillouzouic aguillouzouic deleted the ass_capital branch October 23, 2018 08:12
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.

7 participants