astropy / reproject

Python-based Astronomical image reprojection :milky_way: - maintainer @astrofrog
https://reproject.readthedocs.io
BSD 3-Clause "New" or "Revised" License
105 stars 64 forks source link

Fix multi-threaded reprojection when using Astropy WCS #434

Closed astrofrog closed 2 months ago

astrofrog commented 3 months ago

To run just the relevant test:

tox -e test -- -k "test_dask_scheduler"

This currently shows three test failures:

FAILED ../../.tox/test/lib/python3.11/site-packages/reproject/tests/test_high_level.py::test_dask_schedulers[threads-reproject_interp] - AssertionError: 
FAILED ../../.tox/test/lib/python3.11/site-packages/reproject/tests/test_high_level.py::test_dask_schedulers[threads-reproject_adaptive] - AssertionError: 
FAILED ../../.tox/test/lib/python3.11/site-packages/reproject/tests/test_high_level.py::test_dask_schedulers[threads-reproject_exact] - AssertionError: 

To run just one of the parameter combinations of the test, one can do e.g.:

tox -e test -- -k "test_dask_sched and interp and threads"
codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.63%. Comparing base (2dedbc5) to head (b42f7c6). Report is 17 commits behind head on main.

Files Patch % Lines
reproject/common.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #434 +/- ## ========================================== + Coverage 93.60% 93.63% +0.02% ========================================== Files 25 25 Lines 892 895 +3 ========================================== + Hits 835 838 +3 Misses 57 57 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

manodeep commented 3 months ago

Should the deep copy only be done for the multi-threading case, or is the copy so fast + low-memory that it does not matter

astrofrog commented 2 months ago

@manodeep - the deep copy only takes ~10µs for a typical WCS, so it will be buried in the noise

astrofrog commented 2 months ago

The fix here is a workaround but it is acceptable enough to just go ahead and merge, as even though it does not fix astropy upstream, it should basically make multi-threading usable here.

astrofrog commented 2 months ago

Let's get this fix in 🚀