Skip to content

Fix various vectorial parameter access and add unit tests #1336

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 15, 2025
Merged

Fix various vectorial parameter access and add unit tests #1336

merged 6 commits into from
Oct 15, 2025

Conversation

@benjello
Copy link
Member

@benjello benjello commented Oct 1, 2025 •
edited by guillett
Loading

Technical changes

  • Fix various vectorial parameter access and add unit tests
benjello
benjello commented Oct 1, 2025
View reviewed changes
Copy link
Member Author

@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.

Related to openfisca/openfisca-france#2587

cc @clallemand
@benjello benjello mentioned this pull request Oct 1, 2025
Open
@guillett
Copy link
Member

guillett commented Oct 1, 2025

Oupsyyy
Merci @benjello pourrais-tu ajouter un test ?
@guillett
Copy link
Member

guillett commented Oct 1, 2025

Et désolé @clallemand
@guillett
Copy link
Member

guillett commented Oct 8, 2025

Je ne crois pas que ça soit lié à ça cf. openfisca/openfisca-france#2589

J'ai tenté de faire un test en plus de tests/core/parameters_fancy_indexing/test_fancy_indexing.py:test_with_properties_starting_by_number mais sans succès.

Étant donné que ça n'a pas l'air d'être ça le problème, je ne regarde pas plus pour le moment.
Pour moi, on peut faire cette PR.
@guillett
Copy link
Member

guillett commented Oct 8, 2025

Désolé, en fait, dans un premier temps, je n'avais vu qu'une partie des tests en échec.

Je viens de corriger cette PR en ajoutant un test.

J'ai lancé les tests mais il reste encore des cas à traiter côté France et CSS.
@benjello
Copy link
Member Author

benjello commented Oct 9, 2025

@clallemand est-ce que l'on merge et on refera des tests sur france plus tard ?
@guillett
Copy link
Member

guillett commented Oct 9, 2025

Non non, ça passe pas encore sur France. Je vais avancer là dessus rapidement.

J'ai pas encore des tests sur core mais je vois ceux qui échouent sur France.
@clallemand
Copy link
Contributor

clallemand commented Oct 13, 2025

Bonjour, merci de gérer cette correction. Je devrais être davantage réactive s'il faut tester des choses maintenant !
@guillett guillett force-pushed the fix_vectorial_parameter branch from d292def to 19ce7dc Compare October 15, 2025 10:26
@guillett guillett changed the title Fix vectorial parameter access with integer keys Fix various vectorial parameter access and add unit tests Oct 15, 2025
guillett
guillett approved these changes Oct 15, 2025
View reviewed changes
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.

@benjello can you merge this PR if you're ok with my contributions?
@guillett
Copy link
Member

guillett commented Oct 15, 2025

I locally tested this PR with OpenFisca France test base and all tests passed.
@benjello benjello merged commit f0bbc79 into master Oct 15, 2025
23 checks passed
@benjello benjello deleted the fix_vectorial_parameter branch October 15, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@benjello @guillett @clallemand