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

Add support for tiling FITS studies #30

Closed astrofrog closed 3 years ago

astrofrog commented 3 years ago

This is very much a WIP for now but the goal is to make the toasty commands work seamlessly with FITS files. To start with I wanted to find the minimal changes that would allow tile-study to 'work' with FITS files - to do this I had to introduce a new property called format on PyramidIO and Image, which can be set to the I/O format to use - this is needed because now multiple formats (in this case Numpy and FITS) could represent F32 or F64 data.

Things to wrap up in this PR:

Things that could be done in follow-up PRs:

Not everything has to be done in this PR of course, but this is just a brain dump.

@pkgw - is there anything else you'd like to see done here?

astrofrog commented 3 years ago

Ok so I think I'm happy with the logic so far which is that Image and PyramidIO both have a default_format attribute that indicates what the format should be if not specified in any load/save methods. The file format is now no longer decided by the mode since there is not a one to one mapping anymore. To tile a FITS study:

toasty tile-study 2MASS_k.fits --placeholder-thumbnail

There may be places where the mode is no longer needed, so will check shortly. There's still a lot to do as indicated in the list in the main description above, but at least the basics are working.

pkgw commented 3 years ago

@astrofrog Great! I hope to review what you've got pretty soon.

codecov[bot] commented 3 years ago

Codecov Report

Merging #30 (56c6d96) into master (d013676) will increase coverage by 0.28%. The diff coverage is 82.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   78.17%   78.45%   +0.28%     
==========================================
  Files          14       14              
  Lines        2190     2242      +52     
==========================================
+ Hits         1712     1759      +47     
- Misses        478      483       +5     
Impacted Files Coverage Δ
toasty/multi_wcs.py 0.00% <0.00%> (ø)
toasty/builder.py 84.61% <80.00%> (+0.71%) :arrow_up:
toasty/pyramid.py 84.92% <80.00%> (-0.30%) :arrow_down:
toasty/image.py 72.52% <80.64%> (+1.73%) :arrow_up:
toasty/cli.py 88.53% <87.50%> (+1.32%) :arrow_up:
toasty/merge.py 95.79% <100.00%> (ø)
toasty/multi_tan.py 92.70% <100.00%> (ø)
toasty/pipeline.py 79.07% <100.00%> (ø)
toasty/study.py 84.48% <100.00%> (+0.13%) :arrow_up:
toasty/toast.py 98.87% <100.00%> (ø)
... and 2 more

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 d013676...7e8cd90. Read the comment docs.

astrofrog commented 3 years ago

@pkgw - I'm still working on this and in particular need to fix the cascade command

astrofrog commented 3 years ago

@pkgw - I'd like to make some slightly more extensive changes but wanted to check with you first. Before this PR, the file format was derived from the mode, so e.g. RGBA = PNG and F32 = Numpy. This doesn't generalize well to a third overlapping format and also doesn't make it easy to support different kinds of RGBA formats easily for instance. As a result in this PR I added the ability to specify the format in a few places but kept the previous code to allow the mode to be set which unfortunately makes it easy to give conflicting options.

I'd like to suggest getting rid of the mode as a way to specify the file format. I think there is still room to allow e.g. RGBA vs RGB but other than that I think we could do without. Concretely, I'm suggesting that e.g.

toasty cascade --start 6 . --type f32

would be replaced with:

toasty cascade --start 6 . --format fits

although in practice for the specific example of cascade we can auto-determine the format (I have this implemented locally).

In general, would you be happy for format to be the primary way of setting the format. I could keep mode but have it always default to None which means the default mode for that format. This then would still allow the mode to be settable in ambiguous cases.

pkgw commented 3 years ago

@astrofrog Yes I think this would be a desirable change to make — the "type" specification was a bit of a hack to work around the fact that we didn't have a "format" concept.

astrofrog commented 3 years ago

@pkgw - assuming the CI passes, I think this is potentially mergeable after review. There's still a fair bit of cleanup and improvements I'd like to do but I also really don't like very large PRs that are hard to review and work on, so maybe you could see if you like what's here for now and I can open up follow-up PRs to improve test coverage and address the other TODOs in the description of this PR?

Having said that the docs need updating a bit, will push some changes so the docs are not inconsistent with the implementation.

astrofrog commented 3 years ago

I pushed some improvements to the docs.

pkgw commented 3 years ago

@astrofrog Awesome, I will try to review this promptly. The docs build failure looks like a linkcheck issue with a site that is genuinely down right now ... it's frustrating/surprising how often the linkchecks fail for these reasons!

pkgw commented 3 years ago

I reran the failed docs job and it works now that the NASA site is back up :+1:

astrofrog commented 3 years ago

@pkgw - I've fixed the docstrings!