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.

Sign up for GitHub

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

Jump to bottom

Implement basic interface to ASDF and GWCS #719

Merged
merged 10 commits into from
Jan 8, 2019
Merged

Implement basic interface to ASDF and GWCS #719

merged 10 commits into from
Jan 8, 2019

Conversation

pllim
Copy link
Collaborator

@pllim pllim commented Nov 15, 2018 •
edited
Loading

This might still need unreleased ASDF and GWCS. It definitely requires Astropy 3.1 or later. But it does work, I propose that we merge this at its current state and improve it in future PRs, as needed. (Limited support is better than zero support?)

Fix #327

cc @hcferguson , @nden , and @drdavella

TODO:

  • Test with some files: FITS with FITS WCS, ADSF with GWCS, ASDF-in-FITS with GWCS
  • Write tests.

Open Questions:

  • AstroImage.load_asdf() -- How to let user specify where data and WCS are in ASDF?
  • No way to load JWST data in a meaningful way without JWST data models hidden within JWST pipeline.

Known issues (future PRs):

  • Compass does not work.
  • WCSAxes does not work. But this could be an issue with the test image rather than the plugin; need a better dataset to be sure.
  • Data cube not fully supported. LineProfile does not fully work.
  • Very limited testing done.
  • Documentation deferred to future PR when this is more stable.

@pllim pllim added this to the 3.0 milestone Nov 15, 2018
@ejeschke
Copy link
Owner

There are edits to two files needed still (@pllim will add them to the PR). Result looks like this with the sample ASDF file we got from @nden:
screenshot from 2018-11-15 10-19-17

@ejeschke
Copy link
Owner

Nice sample image, @nden!

@pllim
Copy link
Collaborator Author

pllim commented Nov 15, 2018

@ejeschke I already added them in second commit and gave you credit. Did I miss something? I still need to make sure the GWCS methods are working as expected though.

@ejeschke
Copy link
Owner

I just sent you some changes to them, that are needed.

@pllim
Copy link
Collaborator Author

pllim commented Nov 16, 2018

Okay @ejeschke , they are added as fourth commit.

@nden
Copy link

nden commented Nov 16, 2018

Thanks for adding support for gwcs @ejeschke and @pllim!
First package to do so!

@pllim pllim mentioned this pull request Nov 20, 2018
Closed
6 tasks
@pllim

This comment has been minimized.

Sign in to view
@ejeschke
Copy link
Owner

I think right now it only works with the ASDF embedded in FITS. I just tested in a new environment with the latest asdf, gwcs and astropy from github. It opens the cropped_asdfinfits2.fits file.

@ejeschke
Copy link
Owner

@pllim, just checked the size of the imaging_wcs.asdf file. It is 2.2KB, whereas the asdf embedded in fits file is 1.4MB. Does the ASDF file really have any data or is it just the WCS?

@drdavella
Copy link

How was that particular ASDF file generated?

@pllim
Copy link
Collaborator Author

pllim commented Dec 1, 2018

@nden gave it to us.

@pllim pllim mentioned this pull request Dec 1, 2018
Merged
@pllim
Copy link
Collaborator Author

pllim commented Dec 1, 2018 •
edited
Loading

Does the ASDF file really have any data or is it just the WCS?

@ejeschke , ah, you might be right. Perhaps the solution here is to add some logic to handle empty or non-image ASDF data. FWIW I did see WCS values changing as I mouse around the empty display.

@pllim pllim force-pushed the add-gwcs branch 2 times, most recently from aed4d81 to de76b09 Compare December 5, 2018 04:25
@pllim
Copy link
Collaborator Author

pllim commented Dec 5, 2018

I got ASDF and ASDF-in-FITS to load in reference viewer and the example viewer. And gave a demo of those features at Astropy Coordination Meeting 2018. 😸

@pllim

This comment has been minimized.

Sign in to view
@ejeschke
Copy link
Owner

ejeschke commented Dec 7, 2018

See "questions" and "known issues" in my original post above, which is updated.

@pllim, can you elaborate on why the named plugins don't work? Any insights?

@ejeschke
Copy link
Owner

ejeschke commented Dec 7, 2018

And can you put a cube file with the problems in the Box ginga folder?

@pllim
Copy link
Collaborator Author

pllim commented Dec 7, 2018

@ejeschke , I am not sure. I have been staring at this for a few days and I need a break. There were a string of traceback and I can make little sense of without code diving. I guess it is the subtleties in the ways that WCS methods are called in different places?

For the cube, it is ngc6946.fits that you gave me.

@ejeschke
Copy link
Owner

ejeschke commented Dec 8, 2018

@pllim, compass and LineProfile seem to be working for me with this PR and ngc6946.fits. WCSAxes does not work with the cube, but I don't think that is related this PR. That is just a case of the code not dealing with the result of a coordinate-to-pixel lookup in a cube.

@pllim

This comment has been minimized.

Sign in to view
@pllim

This comment has been minimized.

Sign in to view
@ejeschke

This comment has been minimized.

Sign in to view
@pllim
Copy link
Collaborator Author

pllim commented Jan 6, 2019

@ejeschke , what is the best way to incorporate #649 to this PR? Would you be interested to submit a PR against this PR?

@ejeschke
Copy link
Owner

ejeschke commented Jan 7, 2019

what is the best way to incorporate #649 to this PR? Would you be interested to submit a PR against this PR?

That makes sense. Can you rebase this and I'll take a shot at it.

@pllim
Copy link
Collaborator Author

pllim commented Jan 7, 2019

I am not sure how to resolve the conflict in Control.py after your refactoring. Is it okay to revert your changes just where the conflict is and then you can fix it properly?

@ejeschke
Copy link
Owner

ejeschke commented Jan 7, 2019

Let me try to resolve it via a PR on your PR.

pllim and others added 9 commits January 7, 2019 12:13
@pllim @ejeschke
Add skeleton framework for GWCS support
d6a7219
@ejeschke
Add simple ASDF loader
a215d37
@pllim @ejeschke
Revert accidental edit on wcs_astropy.py
49edbd5
@ejeschke
More changes for ASDF/GWCS support
106e0fb
@pllim @ejeschke
Allow example to open ASDF file
e107bfb
@pllim @ejeschke
Suppress autocuts error when image has 0 dim.
ff03c38 
Enable loading ASDF-in-FITS via MultiDim.

Fix GWCS display on Cursor plugin.

Support loading pure ASDF file in reference viewer.

No point calculating len(ndims) so many times.

Added stop-gap load_header and removed unused _deg from astropy_ape14

Fill in astropy_ape14 methods and add tests. Remove dead code from astropy WCS.

Fix div by zero error on MultiDim.

Apply fps calc from @ejeschke.
@pllim @ejeschke
Minor updates for debugging
6783788
@pllim @ejeschke
Fix APE14 according to comment from @nden
3341c1a
@pllim @ejeschke
Add change log [docs only]
1915795
@ejeschke
Copy link
Owner

ejeschke commented Jan 7, 2019

@pllim, getting the following warning:
AsdfDeprecationWarning: The module asdf.asdftypes has been deprecated and will be removed in 3.0. Use asdf.types instead.

@ejeschke
Copy link
Owner

ejeschke commented Jan 7, 2019

If checks pass, can you try this out @pllim and see if you agree/grok the changes I pushed?

@ejeschke
Copy link
Owner

ejeschke commented Jan 7, 2019

I think this can be improved a bit, but I suggest doing it in another PR.

@pllim
Copy link
Collaborator Author

pllim commented Jan 8, 2019

@ejeschke , thank you very much for resolving the conflict so gracefully. LGTM! Do you want to take the honor/blame of merging this when tests pass? 😉

getting the following warning

Don't worry about that one.

@ejeschke
Copy link
Owner

ejeschke commented Jan 8, 2019

Do you want to take the honor/blame of merging this when tests pass? 😉

@pllim, FFTM 😜

@ejeschke
Copy link
Owner

ejeschke commented Jan 8, 2019

FFTM==Feel Free To Merge

@pllim pllim merged commit 8e51fd5 into ejeschke:master Jan 8, 2019
@pllim pllim deleted the add-gwcs branch January 8, 2019 21:49
This was referenced Jan 8, 2019
Closed
Merged
Closed
Merged
@pllim pllim mentioned this pull request Feb 26, 2019
Closed
@pllim pllim mentioned this pull request Jul 27, 2019
Merged
@pllim pllim mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejeschke ejeschke Awaiting requested review from ejeschke

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pllim @ejeschke @nden @drdavella