Avoid modifying input pixel coordinates in-place #16459
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.