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

Quickfix for numpy.types as meta indicators #30

Merged
merged 8 commits into from
Nov 23, 2023

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Nov 15, 2023

This PR implements a quick-fix when setting meta-indicators with numpy-types, as are used in pd.DataFrames. The PR also improves handling of None, so that these values are ignored when setting from a dictionary run.meta = {"key": None} and are used as "delete" when using the setter like run.meta["key"] = None.

@danielhuppmann danielhuppmann self-assigned this Nov 15, 2023
@glatterf42
Copy link
Member

The tests all seem to pass in the GHA, so what exactly is not working with Run.meta[<key>] = value? Also, what do you want to do and is there no way to use numpy-types for it?

@danielhuppmann
Copy link
Member Author

The tests only pass because of the (very not elegant) casting in setitem. And I’m sure they would fail if we added numpy.float to the tests…

Proposal: anyone recommends a more pythonic approach to the casting (or something else), then I implement it and add tests for other numpy types.

@glatterf42
Copy link
Member

glatterf42 commented Nov 15, 2023

This question seems to be very relevant here.
One option would be to have a map, like you say, but if that map is not created automatically, it might be tedious to cover all cases (because you'd probably want to catch np.float32 and np.float64 and some others as float).
Another suggested answer is something like

if isinstance(value, np.generic): 
    value = value.item()           

This approach should catch all simple data types, but might not work for np.ndarrays or so. Also, some other answers claim it's not the fastest way.
So yet another approach might be

if isinstance(value, np.generic): 
    value = value.tolist()  

Or maybe, this could even be a single line:

value = getattr(value, "tolist", lambda: value)()

Here, tolist() would be called on numpy values, but in case that fails, (since we already have default python type), the lambda function would simply return this value.

@danielhuppmann
Copy link
Member Author

Thanks @glatterf42, followed one of your suggestions...

I also added handling of np.nan/None and Run.meta["<some key>"] = None can now be used to delete an entry.

For explanation, having a dictionary with None-values may seem non-sensical, but it is a real issue when you have a pyam.IamDataFrame where one scenario (=run) does not have values for some indicators. See IAMconsortium/pyam#797 for where this is relevant.

@danielhuppmann danielhuppmann marked this pull request as ready for review November 17, 2023 15:15
ixmp4/core/run.py Outdated Show resolved Hide resolved
ixmp4/core/run.py Outdated Show resolved Hide resolved
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Apart from the requested deletions, this looks good to me. Just one clarifying question: is it enough to test np.float64 or should we investigate why np.float128 is not working and fix that?

@danielhuppmann
Copy link
Member Author

Sorry, removed the print statements.

About numpy.float128, this is converted to a longdouble, wich then creates issues in the database layer. And numpy.float32 is not "precise", writing 13 to the db and reading it again returns 12.9...

Given that pandas uses float64 internally as a default, this PR is good enough for proceeding with the pyam integration. We can tackle more cases once it's necessary.

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, looks good to me, then :)

Copy link
Contributor

@meksor meksor left a comment

Choose a reason for hiding this comment

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

lgtm

@danielhuppmann danielhuppmann merged commit ef94bda into iiasa:main Nov 23, 2023
8 checks passed
@danielhuppmann danielhuppmann deleted the meta/numpy-int64 branch November 23, 2023 11:32
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.

None yet

3 participants