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

Simple entrypoint & multi_tan detection #68

Merged
merged 16 commits into from
Nov 15, 2021

Conversation

imbasimba
Copy link
Member

  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 WorldWideTelescope/pywwt#316

@imbasimba
Copy link
Member Author

@imbasimba
Copy link
Member Author

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?

Copy link

@pkgw pkgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! I have some suggestions below.

I pushed a commit to your branch to see if it helped to upgrade to Python 3.9 for the coverage test, but it looks like it didn't.

setup.py Outdated Show resolved Hide resolved
toasty/collection.py Outdated Show resolved Hide resolved
toasty/simple_interface.py Outdated Show resolved Hide resolved
toasty/simple_interface.py Outdated Show resolved Hide resolved
toasty/simple_interface.py Outdated Show resolved Hide resolved
toasty/simple_interface.py Outdated Show resolved Hide resolved
toasty/simple_interface.py Outdated Show resolved Hide resolved
toasty/simple_interface.py Outdated Show resolved Hide resolved
@pkgw
Copy link

pkgw commented Nov 9, 2021

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
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

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

❗ 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%) ⬇️
toasty/collection.py 57.85% <94.73%> (+6.39%) ⬆️
toasty/merge.py 94.48% <100.00%> (+0.08%) ⬆️
toasty/study.py 94.30% <0.00%> (+0.81%) ⬆️
toasty/image.py 76.72% <0.00%> (+0.92%) ⬆️
toasty/multi_tan.py 83.76% <0.00%> (+5.19%) ⬆️

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
Copy link
Member Author

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. interrupted by signal 11: SIGSEGV set_extent function SciTools/cartopy#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
Copy link

pkgw commented Nov 15, 2021

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

@pkgw
Copy link

pkgw commented Nov 15, 2021

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

@pkgw
Copy link

pkgw commented Nov 15, 2021

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.

As per the comment added to `azure-build-and-test.yml`: normally we try
to pull in as many deps as possible using `pip` to test out our internal
metadata, but this doesn't work well for the coverage run. Rearrange it
to use conda to pull in all of the dependencies.
@pkgw pkgw merged commit 52ab512 into WorldWideTelescope:master Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants