allow override of wcs when computing sky region in roi_subset_state_to_region

[cshanahan1]

In JDaviz (Cubeviz, specifically), sometimes subsets don't contain spatial wcs information in subset.xatt.coords, which is what is used for the pixel-to-sky conversion in roi_subset_state_to_region. To fix this issue, this PR adds the option to provide a different WCS for the conversion, which will override anything in .coords.

(See https://github.com/spacetelescope/jdaviz/pull/2496)

[cshanahan1]

Looks like I pushed some commits i made before seeing the most recent review comments - I added tests (and moved the old one that tested to_sky) and implemented @pllim of overloading to_sky to allow a new WCS to override anything attached to the subset.

[dhomeier]

Thanks, tests look good! I still would just like to see the exception for an invalid or missing wcs in .coords (the original issue above) tested.

[pllim]

Ideally this should perhaps even use two different WCS to make sure the user-provided is used

Let's not do that. Ideally, subset state should always refer to its parent. But in Cubeviz, we have to hack around the fact that cube ingested into Cubeviz somehow contains .coords without the original spatial WCS info (I cannot find that issue for cross-linking right now). So ideally when this problem is fixed properly, no one will ever have to pass in to_sky as a WCS ever again.

[pllim]

Actually.... I just realized that you do not really need this PR, @cshanahan1 . Since you only use this function to compute pixel region, and then convert to sky region internally in Jdaviz, given that you want to keep both pixel and sky.

😬

[dhomeier]

Let's not do that. Ideally, subset state should always refer to its parent. But in Cubeviz, we have to hack around the fact that cube ingested into Cubeviz somehow contains .coords without the original spatial WCS info

Thanks for the background – that explains why it all appears a bit hackish atm :D! So you think no test for invalid input is needed either?

[pllim]

I actually don't think this PR is needed at all. I am going to revisit https://github.com/spacetelescope/jdaviz/pull/2496 shortly.

[dhomeier]

Thanks; keep me posted! I think we may still want to keep some of the extra test coverage and doc updates.

[cshanahan1]

I parameterized the tests which is also something we can keep even if we don't move forward with allowing 'to_sky' to be a WCS.

[cshanahan1]

I'm going to open a PR on a new branch that just has the test + documentation changes we want to keep from this, and close this PR (but keep it separate in case we want to revisit this)

[cshanahan1]

Closing this for now, doc + test changes we want to keep are in https://github.com/glue-viz/glue-astronomy/pull/97