Skip to content
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

Refactor blocked/parallel reprojection #374

Merged
merged 14 commits into from
Sep 7, 2023

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Jul 13, 2023

This refactors the handling of blocked/parallel reprojection with dask following a conversation with @Cadair.

  • Ensure that we clean up all temporary directories with Numpy memmaps and zarr arrays
  • Add a return_type option to reproject_interp which can be 'numpy' (default) or 'dask'. When the latter is used, nothing is computed when reproject_interp is called so this is very fast
  • Document that to use schedulers other than synchronous or processes, users should set return_type to 'dask' and then compute it with the desired scheduler (e.g. a dask distributed scheduler). However, we need to make it very clear that the code is not thread-safe, so that multi-threaded schedulers can't be used (I don't think there is a way for us to check that)
  • Expand the blocked/parallel reprojection to the adaptive and spherical intersect algorithms
  • Make sure we properly test roundtrip_coords and order - these were not getting passed through to the blocked/parallel reprojection so were getting ignored, but need to add tests to make sure these options do something

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #374 (1b8b7c0) into main (1432cec) will decrease coverage by 0.49%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main     #374      +/-   ##
==========================================
- Coverage   93.86%   93.37%   -0.49%     
==========================================
  Files          24       25       +1     
  Lines         847      861      +14     
==========================================
+ Hits          795      804       +9     
- Misses         52       57       +5     
Impacted Files Coverage Δ
reproject/interpolation/core.py 96.61% <ø> (+1.08%) ⬆️
reproject/adaptive/core.py 95.91% <83.33%> (-1.86%) ⬇️
reproject/common.py 91.08% <91.08%> (ø)
reproject/adaptive/high_level.py 100.00% <100.00%> (ø)
reproject/interpolation/high_level.py 100.00% <100.00%> (ø)
reproject/spherical_intersect/core.py 98.61% <100.00%> (+0.53%) ⬆️
reproject/spherical_intersect/high_level.py 100.00% <100.00%> (ø)
reproject/utils.py 83.33% <100.00%> (-3.68%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Cadair
Copy link
Member

Cadair commented Jul 13, 2023

As mentioned in chat we should have a way of parallelising over independent celestial input chunks as well.

@astrofrog
Copy link
Member Author

@Cadair - I am working on that but will add it in a separate PR. In the mean time, would you be able to review this or are you happy for me to just merge?

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.

2 participants