8000 Fix memory policy warning in io.fits by saimn · Pull Request #14168 · astropy/astropy · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@saimn
Copy link
Contributor
@saimn saimn commented Dec 11, 2022

Fix #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

@saimn saimn added this to the v5.3 milestone Dec 11, 2022
@saimn saimn changed the title Fits memory policy Fix memory policy warning in io.fits Dec 11, 2022
Copy link
Contributor
@mhvk mhvk left a 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!

@saimn
Copy link
Contributor Author
saimn commented Dec 12, 2022

Hopefully future self will have a look at the commit message ;) (5fb0787). I also added a link to the Numpy docs in the comment 👍

@mhvk
Copy link
Contributor
mhvk commented Dec 12, 2022

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...

@mhvk mhvk added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Dec 12, 2022
@mhvk
Copy link
Contributor
mhvk commented Dec 12, 2022

I think there is something wrong with [ci skip] in that it requires jobs that do not actually run... (readthedocs failure is unrelated; rebase should solve that).

saimn and others added 2 commits December 13, 2022 09:00
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
@saimn saimn force-pushed the fits-memory-policy branch from c9daf9a to 32a80a1 Compare December 13, 2022 08:00
@mhvk
Copy link
Contributor
mhvk commented Dec 13, 2022

Thanks! Let's get this in...

@mhvk mhvk merged commit cc6df7d into astropy:main Dec 13, 2022
@saimn saimn deleted the fits-memory-policy branch December 13, 2022 13:18
@pllim
Copy link
Member
pllim commented Dec 23, 2022

No need to backport this?

@pllim
Copy link
Member
pllim commented Dec 23, 2022

I think there is something wrong with [ci skip] in that it requires jobs that do not actually run

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.

@mhvk
Copy link
Contributor
mhvk commented Jan 4, 2023

@saimn - as @pllim asked, should this be backported?

@saimn
Copy link
Contributor Author
saimn commented Jan 6, 2023

Probably does not need to be backported, the warning is hidden in current Numpy versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

memory policy warning in fits with numpy 1.22.0.dev

3 participants

0