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
DEP: Remove deprecated numeric style dtype strings #19539
Conversation
Thanks, would be fine to just put this in. But... if you are to it, now that I see the deprecation comment: it would be much nicer to finish the deprecation and delete the whole branch as was started in gh-16554. The code effectively always did the right thing, the warnings were given. The typical timelin for deprecation is one year/two releases, which the next release will be. And there is no reason to delay in this specific case. |
Is this code even reachable? I can't work out how to trigger the warning. |
Use the exact string as dtype: |
I'm trying this out on 1.19.4 and getting no warning |
Thanks for the feedback! Based on your linked PR, here are the things I think I have to do. Does this make sense / is anything missing from my list?
|
No, I think those are fine, note that the main problem here is the capital letter,
Yeah, that looks right.
You should delete the whole |
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 doing the more complete fix! Just two small things also to make the linting CI happy. It would be nice to have a very brief (single bullet point) release note for this. doc/release/upcoming_changes/README.rst
explains how to do this. Just to note that the deprecation is finalized.
But basically, this is good to go :).
I am currently doing the release note and I am wondering on what PR type I should list this as? |
It would be |
hmm, do you have any idea on why the test is failing @seberg ? I don't know what is going wrong |
Those unfortunately are flaky, don't worry about them. The changes are all good I think. I would modify the release note to something like:
|
ok sounds good but just to clear things up, would I also have to include Uint32 and Uint64 as strings in the release note? |
Yeah, I think listing them all makes sense, it is not that long of a list. |
Cancelled the last job, I suspect things were just slow due to the github issues today. In effect, all jobs have run with only style change differences anyway. Thanks @NectDz, especially for taking the time to finish the deprecation when the issue was just about the smaller fix! |
Finishes the deprecation, and effectively closes numpygh-18993 * Insecure String Comparison * Finished Deprecations * Breaks numpy types * Removed elements in dep_tps * Delete Typecode Comment * Deleted for loop * Fixed 80 characters or more issue * Expired Release Note * Updated Release Note * Update numpy/core/numerictypes.py * Update numpy/core/tests/test_deprecations.py Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
Finishes the deprecation, and effectively closes numpygh-18993 * Insecure String Comparison * Finished Deprecations * Breaks numpy types * Removed elements in dep_tps * Delete Typecode Comment * Deleted for loop * Fixed 80 characters or more issue * Expired Release Note * Updated Release Note * Update numpy/core/numerictypes.py * Update numpy/core/tests/test_deprecations.py Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
See #18993
This issue was presented by @Daybreak2019
When the function strncmp was used it did not account for a null byte character meaning it is an incomplete comparison.
The elements in the array dep_tps on line 1727 in the descriptor.c file did not match the elements in the deprecated_types array in test_deprecations.py line 324 therefore a change in the dep_tps array had to be made so the null byte character could be compared correctly.
The null byte character is compared on line 1732 in the descriptor.c file by the change strlen(dep_tp)+1 which includes the strings length but adds +1 so the null byte character can also be compared making the strncmp function compare both strings completely.