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

Improve usability of Enum values #840

Merged
merged 6 commits into from
Apr 5, 2019
Merged

Conversation

Morendil
Copy link
Contributor

@Morendil Morendil commented Mar 12, 2019

Allow the use of Enum values in comparisons, working around our abuse of the Python module system for TaxBenefitSystem loading.

Tested by updating test_holders.py to import HousingOccupancyStatus instead of getting possible_values. (Also tested, outside of CI, against the YAML test in this branch of France.)

@Morendil Morendil changed the title [WIP] Fix double import errors Improve usability of Enum values Mar 13, 2019
@Morendil Morendil changed the title Improve usability of Enum values [WIP] Improve usability of Enum values Mar 13, 2019
@Morendil Morendil force-pushed the fix-double-import-errors branch from 7fd095f to d60052f Compare March 13, 2019 09:39
@Morendil Morendil changed the title [WIP] Improve usability of Enum values Improve usability of Enum values Mar 13, 2019
@Morendil Morendil force-pushed the fix-double-import-errors branch from 6c2272c to b0e15b0 Compare March 13, 2019 11:40
@sandcha
Copy link
Collaborator

sandcha commented Mar 14, 2019

Might be tested with this code calling openfisca-tunisia :

from openfisca_tunisia import CountryTaxBenefitSystem as TunisiaTaxBenefitSystem
from openfisca_core.simulation_builder import SimulationBuilder

from openfisca_tunisia.model.prelevements_obligatoires.cotisations_sociales import TypesRegimeSecuriteSociale

tax_benefit_system = TunisiaTaxBenefitSystem()

nb_individus = 3
simulation_builder = SimulationBuilder()
simulation = simulation_builder.build_default_simulation(tax_benefit_system, count=nb_individus)

period = '2019-01'
simulation.set_input('salaire_de_base', period, [1000, 1000, 1000])

regimes = [TypesRegimeSecuriteSociale.rtns, TypesRegimeSecuriteSociale.raci, TypesRegimeSecuriteSociale.salarie_cnrps]
simulation.set_input('regime_securite_sociale', period, regimes)

ugtt = simulation.calculate('ugtt', period)
assert ugtt[2] == -3, ugtt[2]

@Morendil
Copy link
Contributor Author

@sandcha That looks like it would work, and OpenFisca Tunisia needs to be updated to Core 25 first.

@Morendil Morendil force-pushed the fix-double-import-errors branch from b0e15b0 to f000f22 Compare April 5, 2019 10:29
Copy link
Collaborator

@sandcha sandcha left a comment

Choose a reason for hiding this comment

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

Thank you @Morendil!
It resolves the initial issue on openfisca-tunisia.

# However, variables (and hence their possible_values) are loaded by a call to load_module,
# which gives them a different identity from the ones imported in the usual way.
# So, instead of relying on the "cls" passed in, we use only its name to check that
# the values in the array, if non-empty, are of the right type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shorten comment or keep in PR only?

Suggested change
# the values in the array, if non-empty, are of the right type.
# "cls" generally comes from variables.possible_values (thus, from load_module)
# while the array values may come from directly importing a module containing an Enum class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you know I'm not a fan of long comments in code, even of comments in general. In this particular case I really feel that the somewhat long explanation is needed to make sense of the code that follows…

@@ -141,6 +141,8 @@ def calculate(self, variable_name, period, **parameters):
if array is None:
array = holder.default_array()

array = self._cast_formula_result(array, variable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@Morendil Morendil merged commit ae1dad2 into master Apr 5, 2019
@Morendil Morendil deleted the fix-double-import-errors branch April 5, 2019 14:41
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.

2 participants