search

ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Pan "hack" causing compass failure for HST/ACS FLC image #960

Closed pllim closed 2 years ago

pllim commented 3 years ago

I am not sure why it is forcing an error here for a big (1 degree) offset.

https://github.com/ejeschke/ginga/blob/833517ab092a525b2eac63b736515a21951dacd2/ginga/rv/plugins/Pan.py#L310-L311

This causes the following failure for HST/ACS FLC image, jbt7a3k7q_flc.fits. The detector has 202x202 arcsec^2 FOV and the image WCS is not corrected for distortion at this stage.

WCSError: 'WCS.all_world2pix' failed to converge to the requested accuracy.
After 2 iterations, the solution is diverging at least for one input point.
ejeschke commented 3 years ago

Hmm, well, I think the hack worked as advertised--if the WCS fails it is not going to be able to calculate a compass. Was this astropy or astropy_ape14 WCS that failed?

pllim commented 3 years ago

astropy_ape14

ejeschke commented 3 years ago

@pllim, I have that file. I can confirm it loads fine and displays a compass with astropy WCS. Compass looks like the one displayed by ds9.

ejeschke commented 3 years ago

Given that astropy is the default WCS, and we won't merge anything till after this release...probably not a blocker for 3.2?

pllim commented 3 years ago

Not a blocker. Thanks!

pllim commented 3 years ago

I don't understand why the hack is there and why it needs such a big offset (1 degree in the sky). When there is distortion, such a big offset outside of the detector is not going to give you anything good.

ejeschke commented 3 years ago

I'm totally ok with trying a smaller offset. I can't remember exactly how the offsets (lengths) for the arms is calculated, but I think this particular call is just to make sure that a WCS calculation can be performed.

ejeschke commented 3 years ago

But I'd still be curious why astropy_ape14 doesn't handle the calculation, whereas astropy does. Does that signal anything significant?

pllim commented 3 years ago

I don't remember the details, but APE 14 is the canonical way now. Perhaps the old way ignored distortion, like DS9, but I am just guessing.

APE 14: https://github.com/astropy/astropy-APEs/blob/main/APE14.rst

ejeschke commented 3 years ago

Hmm, I'd be curious about the time difference between them as well. Sounds like the new one does some sort of iteration to a solution.

ejeschke commented 3 years ago

Is the old way going to be deprecated (in astropy itself, I mean)? Is there a timeline for when we have to merge astropy_ape14 and astropy?

pllim commented 3 years ago

AFAIK, there is no plan to remove any WCS stuff from astropy. However, APE 14 implementation is necessary for GWCS support.