WorldWideTelescope / toasty

Break large images into "tile pyramids", with a focus on the all-sky TOAST format.
https://toasty.readthedocs.io/
MIT License
0 stars 4 forks source link

Simple entrypoint & multi_tan detection #68

Closed imbasimba closed 2 years ago

imbasimba commented 3 years ago
  1. Added a simple interface, to allow external users to use toasty without knowing about toasty internals. Only enter a list of FITS and this function will take care of the rest.
  2. An ImageCollection can now detect if it is multi_tan or multi_wcs.
  3. Also, now requirng shapely and reproject. Are there any good reasons to not to require these dependencies explicitly?

This PR is used in https://github.com/WorldWideTelescope/pywwt/pull/316

imbasimba commented 3 years ago

Hmm... That segmentation fault is interesting. Anything you've seen before, @pkgw? https://dev.azure.com/aasworldwidetelescope/WWT/_build/results?buildId=2211&view=logs&j=9c812e60-d21b-5131-8477-4061cfb57ebe&t=3bef032c-d60d-5a2a-8d25-6ff5ebcaca46

imbasimba commented 3 years ago

Maybe the failing test did not run on Azure before, since it is marked with @pytest.mark.skipif('not HAS_REPROJECT'). This PR installs reproject by default. Maybe there is some rationale to not do this?

pkgw commented 3 years ago

For the shapely issue: I think that your change to setup.py isn't actually relevant here, since the "coverage" job installs all of the bells-and-whistles extra packages that toasty uses — that's what you need to maximize the code coverage of your test suite, after all. You can see in the logs of previous runs that reproject and shapely are available and should have been getting exercised.

That being said ... it's certainly not clear to my why your changes would cause the test_as_multi_wcs test to start failing. And I can't reproduce locally. I guess my next step would be to create a pristine Python environment and follow all of the commands from the CI scripts exactly, to see if I can reproduce if I really try to copy the test setup precisely.

codecov[bot] commented 3 years ago

Codecov Report

Merging #68 (9579c71) into master (d52a58d) will increase coverage by 0.67%. The diff coverage is 91.66%.

:exclamation: Current head 9579c71 differs from pull request most recent head 085f005. Consider uploading reports for the commit 085f005 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   74.69%   75.37%   +0.67%     
==========================================
  Files          22       22              
  Lines        3051     3098      +47     
==========================================
+ Hits         2279     2335      +56     
+ Misses        772      763       -9     
Impacted Files Coverage Δ
toasty/__init__.py 89.28% <88.88%> (-10.72%) :arrow_down:
toasty/collection.py 57.85% <94.73%> (+6.39%) :arrow_up:
toasty/merge.py 94.48% <100.00%> (+0.08%) :arrow_up:
toasty/study.py 94.30% <0.00%> (+0.81%) :arrow_up:
toasty/image.py 76.72% <0.00%> (+0.92%) :arrow_up:
toasty/multi_tan.py 83.76% <0.00%> (+5.19%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d52a58d...085f005. Read the comment docs.

imbasimba commented 3 years ago

Alright! All review comments fixed and all CI issues addressed (sort of). There were three issues discovered in CI, of which two are fully solved and one which is still being investigated.

  1. https://dotnetfoundation.org/ ssl certificate expired yesterday, but it has now been renewed
  2. Segmentation fault caused by interfering installations of libgeos, which comes with Shapely. Since I added Shapely in the dependencies, and it was also installed with conda, these two installations did not play nice with each other. Unfortunately this issue can now pop up for any user who has installed Shapely with Conda. https://github.com/SciTools/cartopy/issues/1277
  3. The latest issue I have stumbled upon is regarding the parallelization during build.cascade. For some, as of yet, unknown reason the cascade run in test_toasty.py seems to continue forever if run in parallel mode. Forcing the test to use serial processing allows the test to pass. Of course, we have to find the real issue with the parallel processing asap, but it is a bit cumbersome on my Windows machine which doesn’t allow toasty parallelization. @pkgw let me know if you see anything weird with the parallel cascading, otherwise I can take a deep dive next week.
pkgw commented 3 years ago

Well, "good" news that I can reproduce the parallel deadlock locally, so it should be quick to fix.

pkgw commented 3 years ago

Ah, yes: the merge task would get stuck when start = 0, i.e. there is no actual work to do!

pkgw commented 3 years ago

Ah, now I see where the shapely issue is coming from. I've force-pushed a new history that ought to fix it (might take a couple of tries) and also tidies up the history regarding the CI issues.