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

Map ITRS frames to terrestrial WCS coordinates #6990

Merged
merged 1 commit into from Mar 6, 2018

Conversation

lpsinger
Copy link
Contributor

This will make it possible to use WCSAxes to make figures that combine both celestial and terrestrial features. An example is plotting the coordinates of an astronomical transient over an all- sky satellite image to illustrate the position relative to the Earth at the time of the event.

The ITRS frame is identified with WCSs that use the TLON- and TLAT- coordinate types. There are several examples of WCSs where this syntax is used to describe terrestrial coordinate systems: Section 7.4.1 of the WCS in FITS "Paper II", and the WCSTools docs: http://tdc-www.harvard.edu/software/wcstools/wcstools.multiwcs.html

This is the technique that I am using to make images such as this one: http://ligo.org/detections/GW170817/images-GW170817/skymap+earth.jpg

@astropy-bot
Copy link

astropy-bot bot commented Dec 14, 2017

Hi there @lpsinger 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@pllim pllim added the wcs label Dec 15, 2017
@pllim pllim added this to the v3.0.0 milestone Dec 15, 2017
@pllim pllim requested review from nden and astrofrog December 15, 2017 15:34
xcoord = 'TLON'
ycoord = 'TLAT'
wcs.wcs.radesys = 'ITRS'
wcs.wcs.date_obs = frame.obstime
Copy link
Contributor

Choose a reason for hiding this comment

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

The attribute is wcs.wcs.dateobs. I think this will fail with AttributeError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are! Fixed.

@nden
Copy link
Contributor

nden commented Dec 22, 2017

The image is neat!

I left one inline comment. I wonder what woul dbe a test for this - perhaps roundtripping with a coordinates frame?

@lpsinger
Copy link
Contributor Author

I left one inline comment. I wonder what woul dbe a test for this - perhaps roundtripping with a coordinates frame?

Would an image example in the astropy.visualization.wcsaxes documentation count as a test?

@nden
Copy link
Contributor

nden commented Jan 12, 2018

@lpsinger Regarding the test, I was thinking about purely converting between coordinates.ITRS and wcs and back to ITRS. For example, looking at the code I'm not sure how this line will work

wcs.dateobs = itrs.obsdate

The WCS attribute expects a string and it is given the Time object. Should it be assigned str(itrs.obstime) instead?

@lpsinger
Copy link
Contributor Author

@lpsinger Regarding the test, I was thinking about purely converting between coordinates.ITRS and wcs and back to ITRS.

OK, I added unit tests.

For example, looking at the code I'm not sure how this line will work

wcs.dateobs = itrs.obsdate
The WCS attribute expects a string and it is given the Time object. Should it be assigned str(itrs.obstime) instead?

Good catch! Fixed.

@bsipocz bsipocz modified the milestones: v3.0.0, v3.1 Jan 17, 2018
@lpsinger
Copy link
Contributor Author

The CI failures seem to be unrelated to this patch.

Copy link
Contributor

@nden nden left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@nden
Copy link
Contributor

nden commented Mar 1, 2018

@lpsinger Could you rebase again on astropy master. This will pick up some recent fixes to the astropy build and hopefully fix the tests errors (which are unrelated) but for some reason they don't go away. Sorry this is dragging for such a long time.
Let me know if you don't have time and I'll rebase it on my side.

@lpsinger
Copy link
Contributor Author

lpsinger commented Mar 1, 2018

@lpsinger Could you rebase again on astropy master.

Done.

@nden nden added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Mar 1, 2018
@lpsinger
Copy link
Contributor Author

lpsinger commented Mar 1, 2018

The CI failures still look unrelated.

@nden
Copy link
Contributor

nden commented Mar 1, 2018

Yeah, this is the common failure now on master due to numpy 1.14. Thanks for rebasing.

@nden
Copy link
Contributor

nden commented Mar 6, 2018

This needs one more rebase, sorry about that.

This will make it possible to use WCSAxes to make figures that
combine both celestial and terrestrial features. An example is
plotting the coordinates of an astronomical transient over an all-
sky satellite image to illustrate the position relative to the
Earth at the time of the event.

The ITRS frame is identified with WCSs that use the `TLON-` and
`TLAT-` coordinate types. There are several examples of WCSs where
this syntax is used to describe terrestrial coordinate systems:
Section 7.4.1 of the WCS in FITS "Paper II", and the WCSTools docs:
http://tdc-www.harvard.edu/software/wcstools/wcstools.multiwcs.html

This is the technique that I am using to make images such as this one:
http://ligo.org/detections/GW170817/images-GW170817/skymap+earth.jpg
@lpsinger
Copy link
Contributor Author

lpsinger commented Mar 6, 2018

Looks like the tests are passing now! 🎉

@nden
Copy link
Contributor

nden commented Mar 6, 2018

🎉 Thank you. Merging as coverage does not work and everything else is green.

@nden nden merged commit 5b3e07f into astropy:master Mar 6, 2018
@lpsinger lpsinger deleted the terrestrial-wcs branch March 6, 2018 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wcs 💤 merge-when-ci-passes Do not use: We have auto-merge option now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants