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

Updates to match the NEP #95

Merged
merged 12 commits into from
Nov 20, 2023
Merged

Updates to match the NEP #95

merged 12 commits into from
Nov 20, 2023

Conversation

ngoldbaum
Copy link
Member

This is mostly cosmetic changes to match some of the naming I arrived at in the NEP. I also fixed a reference counting issue in stringdtype_setitem that caused a memory leak I hadn't noticed until now.

I also updated the README. @rgommers is there any chance you can try this out in a fresh python environment and let me know if you run into issues? I'd like to include a call for testing the dtype in my message to the numpy mailing list about the NEP updates.

@ngoldbaum
Copy link
Member Author

ngoldbaum commented Nov 17, 2023

@seberg in the last few commits I refactored it so npy_packed_static_string is an opaque struct. I also added NpyString_pack to mirror NpyString_load and used it in a number of places where it made sense to do so.

The only slightly tricky thing is the dtype implementation needs to know the struct size and alignment to set the elsize and alignment, so I defined a macro that handles that. The macro and the layout just need to be kept in sync if they're ever changed.

@ngoldbaum
Copy link
Member Author

OK, this now fully matches what I have in the latest version of the NEP. Pulling this in.

@ngoldbaum ngoldbaum merged commit 01b2245 into numpy:main Nov 20, 2023
1 check 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.

1 participant