search

ratt-ru / tigger-lsm

Python libraries and command-line tools for manipulating Tigger LSMs
2 stars 8 forks source link

New Tigger version workaround and WCS changes #28

Closed razman786 closed 3 years ago

razman786 commented 3 years ago

Following on from the issue here https://github.com/razman786/tigger_lsm_pyqt5/issues/15 this is a workaround patch for tigger-lsm based on some empirical testing with DS9. @bennahugo - hopefully this is useful for you from previous discussions.

Please note getting some of these values to align with a pointer is tricky for me on a laptop. Also, on occasion the pointer in the screenshot is not in its true location, please check only using the coordinate values for comparison.

Small image (star_model_image.fits) - DS9 reference:

small_ds9_reference

Small image with tigger-lsm pre-WCS changes:

small_prewcs_tigger_lsm

Small image with tigger-lsm and this PR post-WCS changes:

small_tigger_lsm_upstream_with_new_fix

Large image (A3528N-delay-MFS-image.fits) - DS9 reference:

large_ds9_reference

Large image with tigger-lsm pre-WCS changes:

large_prewcs_tigger_lsm

Large image with tigger-lsm and this PR post-WCS changes:

large_tigger_lsm_upstream_with_new_fix

Just as additional information which I have not concluded but think it could be useful:

Two large images with tigger-lsm pre-WCS:

2_large_images_prewcs_tigger_lsm

Two large images with tigger-lsm and this PR post-WCS:

2_large_images_tigger_lsm_upstream_with_new_fix

ratt-priv-ci commented 3 years ago

Can one of the admins verify this patch?

razman786 commented 3 years ago

@bennahugo - Found a case where this workaround doesn't work.

New test case with tigger-lsm pre-WCS changes:

new_test_tigger_lsm_prewcs

New test case with tigger-lsm and this PR post-WCS:

new_test_tigger_lsm_upstream_with_new_fix

The following line from SkyImage.py produces a NaN result:

l0, m0 = proj.lm(ra0, dec0)

Which calls the following from tigger-lsm, FITSWCS:

def lm(self, ra, dec):
    l, m = super().lm(ra, dec)
    return sin((l - self._l0) * self.xscale), sin((m - self._m0) * self.yscale)

Output with some debug statements:

setPlotProjection before proj.lm(ra0, dec0) ra0 -1.3089969389957472 dec0 0.3490658503988659
setImageCoordinates (256, 256, 128.0, 128.0, nan, nan, 6.302577854382223e-06, 6.302577854382223e-06)
lmPix l 0.39482893898053, m -0.006748355083077684 _x0 128.0 _l0 nan _dl 6.302577854382223e-06 _y0 128.0 _m0 nan _dm 6.302577854382223e-06

This will need further investigation.

razman786 commented 3 years ago

Just an FYI, astropy world2pix returns NaN by design when the values don't converge. I fixed this but the offset is still incorrect.

razman786 commented 3 years ago

New test case looks no different to the image results here https://github.com/razman786/tigger_py5/issues/166

With updates it now looks like this, obviously it still has the projection bug from the link above, but is now consistent:

new fixed

new_fixed_single_image_verify

N.B. The test failure is from scipy 1.6.2 not being available for Python 3.6, max version is 1.5.4 for Python 3.6

bennahugo commented 3 years ago

Thanks @razman786. I've figured the precision issue out betwen 2.7 and 3.x, it was actually luckily not an internal representation issue. I can now at least roll it back and forth inside tigger2.7 to find the root of the issue.

razman786 commented 3 years ago

@bennahugo - perfect!! I've been doing some testing and using astropy to do some comparisons. I'll ping you with what I've found.

razman786 commented 3 years ago

Closing this as PR #31 has fixed this.