-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: warn when saving dtype with metadata #14994
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
Conversation
Would recommend a recursive strategy: def _has_metadata(dt):
if dt.metadata is not None:
return True
elif dt.names is not None:
return any(_has_metadata(dt[k]) for k in dt.names)
elif dt.subdtype is not None:
return _has_metadata(dt.base)
else:
return False |
My main question is whether we should give a warning, or rather just raise right away. The error/warning message could possibly include Erics recursive version looks good to me. |
Possibly with a comment saying "good luck building a suitable dtype from your nested structured type while preserving field order and padding"... I think the only viable options available to users are:
|
Fair point, using |
This seems like the best option for 1.18, adding a subclass |
warnings.warn("metadata on a dtype may be saved or ignored, but will " | ||
"raise if saved when read. Use another form of storage.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a tracking issue we can reference regarding making this work properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened gh-15017
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last question - do we want to reference that issue in the warning message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we fix the issue the test will fail, which should bring us back to the warning. I think that is sufficient for this corner case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more for pointing users to more information. Fine as is, just curious if putting issue numbers in warnings is a path we should look at going down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a good point, for now I think I will just copy the full warning text into the issue. Search engines will probably pick it up well.
Closing and reopen, to re-trigger failing tests. |
@seberg Ping. |
Thanks Matti and Eric! |
Address gh-14142 for the 1.18 release: warn when saving a dtype with metadata that cannot be loaded.