-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix memory policy warning in io.fits #14168
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
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.
This looks good. My tendency would be to include your explanation above in the comment you already included - I think anybody else (or indeed your future self) looking at the code will thank you!
|
Hopefully future self will have a look at the commit message ;) (5fb0787). I also added a link to the Numpy docs in the comment 👍 |
|
I fear astropy is so inconsistent with its commit messages that I at least have never looked at any -- though I do go to relevant github issues if those are linked. Anyway, in the code it will be seen for sure! Happy to have this merged, but I don't think I am allowed to before the tests have passed... |
|
I think there is something wrong with |
Fix astropy#12324 : Fix the warnings emitted by Numpy when transferring ownership by setting ``NPY_ARRAY_OWNDATA``: RuntimeWarning: Trying to dealloc data, but a memory policy is not set. If you take ownership of the data, you must set a base owning the data (e.g. a PyCapsule). The warning was added in [1], and is now visible only when setting the NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details and a fix see [3]. [1] numpy/numpy#17582 [2] numpy/numpy#20200 [3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set
c9daf9a to
32a80a1
Compare
|
Thanks! Let's get this in... |
|
No need to backport this? |
Nothing wrong per se: "ci skip" and "required" are from two different things that sometimes do not play well together. "ci skip" asks CI not to run. "required" is from repo branch protection to make sure important CI jobs are passing before a maintainer is allowed to merge. This was discussed during infrastructure tag-up a while back. We decided the benefit of requiring a bunch of jobs to pass outweighs the inconvenience of "ci skip" requiring an admin to overwrite. |
|
Probably does not need to be backported, the warning is hidden in current Numpy versions. |
Fix #12324 : Fix the warnings emitted by Numpy when transferring
ownership by setting
NPY_ARRAY_OWNDATA:The warning was added in [1], and is now visible only when setting the
NUMPY_WARN_IF_NO_MEM_POLICY environment variable [2]. For more details
and a fix see [3].
[1] numpy/numpy#17582
[2] numpy/numpy#20200
[3] https://github.com/numpy/numpy/blob/main/doc/source/reference/c-api/data_memory.rst#what-happens-when-deallocating-if-there-is-no-policy-set