-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Avoid modifying input pixel coordinates in-place #16459
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
94555f2
to
9a6a2ce
Compare
Adding WCS benchmarks here: astropy/astropy-benchmarks#118 |
…d coordinates since this isn't thread-safe. Instead, make a copy of the wcsprm and offset crpix. Even when no offset is needed (e.g. if origin==1) it is a good idea to copy the wcsprm since we modify the NaN values in-place to change them to a WCSLIB-specific value.
9a6a2ce
to
3a8b38e
Compare
As highlighted in #16244, modifying input pixel arrays in the WCSLIB C wrapper is not thread-safe. There are two possible solutions:
wcsprm
, offsetcrpix
, and then use that for the conversion. This has the added benefit that we also avoid modifying thewcsprm
in-place to change NaN values to WCSLIB'sUNDEFINED
and back, which also has the risk of causing issues in a multi-threaded context (for instance if one thread changesUNDEFINED
back toNaN
while another thread is relying on the values beingUNDEFINED
).WCS
Python API before passing them to e.g.wcs.wcs.p2s
. Currently if one callswcs_pixel2world
with(x, y, 0)
, a copy is made anyway to stackx
andy
. But in the case where it is called with(xy, 0)
, a copy is not made if the data is contiguous and of floating-point type. We could check for this case and force a copy.we could change this line to use
PyArray_FromArray
and pass a flag to force a copy, always.Option 1 is this PR, although there are other places where this needs to be fixed, but I am waiting to see if we go with this option. It does mean copying the
wcsprm
but this should have a small memory footprint.Option 2 is not really optimal because (a) users might call the
astropy.wcs.Wcsprm
methods directly in which case the unsafe behavior will continue, and (b) because it is nice to have a way to call the conversion routines which doesOption 3 should be safe and would require the fewest changes in lines of code, though it would increase memory usage, and would in particular result in unnecessary copies in some cases.
Performance-wise, Option 1 doesn't seem to be worse than the current code in
main
- we do require a copy ofwcsprm
but on the other hand we are not offsetting pixel coordinates. I'll be sure to add some benchmarks to astropy-benchmarks now though.Anyway at this point, let's discuss if we like Option 1, and if so I can extend it to other parts of
wcslib_wrap.c
.