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

Rewrite how to evaluate booleans in enum creation process #205

Merged
merged 3 commits into from
May 8, 2024

Conversation

anth-volk
Copy link
Contributor

@anth-volk anth-volk commented May 7, 2024

  • make format && make documentation has been run.

What's changed

Fixes #203. Following the merging of #188, policyengine-uk variables of type enum consistently produced strange behavior when loaded as part of a microsimulation, seemingly being weighted correctly, but against an array populated with default values, as opposed to the expected distribution.

Manual testing found that, confusingly, the code introduced in #188 resulted in every enum being loaded twice - once as a string-type enum that was parsed into a single-element array of [0], and then again as an enum-type array that for some reason extended this array out to the same length as the weighting parameter. This contrasts with the previous behavior, whereby most enums (e.g., the variable gender in policyengine-uk) would be read only as a string-type enum, creating an array of correct length with correct values at that time.

Testing eventually demonstrated that the cause of this was the line from #188 that replaced the prior code's check of whether or not an enum's value array is Boolean-type to a different method of assessing this. When this line was replaced, without changing anything else, the code functioned correctly for policyengine-uk, and appears to function correctly for policyengine-us, as well, though my testing on this was less thorough.

That said, being frank, I don't know why this is the solution. It took quite a while to even find it, considering how seemingly unrelated it is to the problem. I'm also at a bit of a loss as to how to test it more substantively. That said, the code does pass all existing tests. This PR also adds ".env" to the gitignore file.

Bug fix

  • Regression test added
  • Regression test passing

@MaxGhenis
Copy link
Contributor

MaxGhenis commented May 7, 2024

We can merge this as a patch, but it does reopen #100

https://github.com/PolicyEngine/policyengine-core/actions/runs/8992822145/job/24703453743?pr=205#step:5:34

/home/runner/work/policyengine-core/policyengine-core/policyengine_core/enums/enum.py:53: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
if isinstance(array == 0, bool):

Any way to get it working without this regression?

Copy link
Contributor

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

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

Thanks- merging given the severity of the UK issue and will reopen #100.

@nikhilwoodruff nikhilwoodruff merged commit d6fa6a1 into PolicyEngine:master May 8, 2024
4 checks passed
@anth-volk anth-volk deleted the fix/fix_enums branch May 8, 2024 12:49
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.

Possible bug in loading Enum microdata
3 participants