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

Replace numpy-Python comparison with dtype #210

Merged
merged 2 commits into from
May 13, 2024

Conversation

anth-volk
Copy link
Contributor

  • make format && make documentation has been run.

What's changed

Fixes #100. Previously, in #205, we downgraded one line of enums.py. The reason for this line was previously unclear, but notably, it always runs: it takes the array variable, determines whether it equals 0 (which yields True or False), then determines whether the resulting value is a Boolean, which it always is.

This was construed as a means of checking whether or not an array is of Boolean type in the comments, but what this piece of code actually does is converts all arrays passed into the function to Numpy's Unicode-string type. However, that is ambiguous due to how it's written, as well as Numpy's own confusing syntax. I believe this code also raises a FutureWarning because it takes a Numpy ndarray type and compares it with a Python type, in this case, a number.

In order to silence the warning and clean up the code, this PR instead determines whether the inbound array is of dtype "S," which confusingly stands for "bytewise-string." If so, it converts it to "str" type, which yields a dtype "U," or "Unicode-string," array.

Tests

This was tested via a Jupyter notebook with a sample policyengine-uk setup that mirrors Nikhil's in #203. That said, it has not been tested with policyengine-us.

Copy link
Contributor

@MaxGhenis MaxGhenis left a comment

Choose a reason for hiding this comment

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

could we add a test?

@anth-volk
Copy link
Contributor Author

Test has been added

@nikhilwoodruff nikhilwoodruff merged commit de4b868 into PolicyEngine:master May 13, 2024
4 checks passed
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.

FutureWarning regarding bool checking in enum.py
3 participants