-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
ENH: drop dtype metadata #23371
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
ENH: drop dtype metadata #23371
Conversation
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.
Thanks for the PR. It looks great, just a few small suggestions.
numpy/lib/utils.py
Outdated
l.append(( | ||
n, # name | ||
drop_metadata(t[0]), # format | ||
t[1], # offset | ||
t[2] if len(t) == 3 else None, # title | ||
)) |
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.
l.append(( | |
n, # name | |
drop_metadata(t[0]), # format | |
t[1], # offset | |
t[2] if len(t) == 3 else None, # title | |
)) | |
if len(t) == 3: | |
l.append((n, drop_metadata(t[0]), t[1], t[2])) | |
else: | |
l.append((n, drop_metadata(t[0]), t[1], None)) |
I think this is much more readable.
numpy/lib/utils.py
Outdated
@@ -1140,4 +1140,31 @@ def _opt_info(): | |||
enabled_features += f" {feature}?" | |||
|
|||
return enabled_features | |||
|
|||
def drop_metadata(dt): | |||
align = dt.isalignedstruct |
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.
align = dt.isalignedstruct |
This variable is unused, so it should be removed.
@ninousf sorry for not following up earlier? Did you have a use-case beyond dropping metadata in |
Hi Sebastian |
It looks a bit like we may add One thing I am wondering: I somewhat like the idea of adding the behavior:
That would allow us to do:
Although, I am not sure we actually need the warning there, maybe it is useful to be able to know that a change happened? |
856ba1a
to
e2cbe53
Compare
@ninousf could you review my changes? I made some heavier changes to use this for Now, I don't like The new code is a bit more robust than the previous one, because it returns the original dtype when there is no metadata attached. So at least if no metadata is used things should be fine even for user dtypes. |
@seberg it's ok for me, thanks |
This could use a release note. |
@ninousf This needs a release note. If you cannot add that today, I'm going to push this off to the next release. |
@charris Tell me if it's ok for you |
Looks fine, thanks. |
Thanks @ninousf |
Please consider marking this as fully public. Metadata is great to forego ndarray subclasses, but until this function came along, there was no good way to e.g. update a value in the metadata. PS: Actually, maybe consider changing the order of the metadata update, so that all |
the continuation of the PR #23185
(Doesn't actually fix the
descr
part including things, but the important thing here)Closes gh-23169, gh-15488