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

New Tigger version workaround and WCS changes #28

Closed
wants to merge 6 commits into from

Conversation

razman786
Copy link
Contributor

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
Copy link

Can one of the admins verify this patch?

@razman786
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

refsky, refpix and x/yscale to use WCS differently. The recenter test will fail, but this
version works for the new Tigger...
razman786 added a commit to razman786/tigger-lsm that referenced this pull request May 10, 2021
related to PR ratt-ru#28. Provides a
self-contained Tigger v1.6.0 workaround.
bennahugo added a commit that referenced this pull request May 13, 2021
* Fixes issue #30 and is
related to PR #28. Provides a
self-contained Tigger v1.6.0 workaround.

* Updated PR following comments here #31

* FITSWCS now inherits from _Projector directly. Astropy methods are implemented for lm() and radec() methods, and is used to calculate refpix.

Test file model/2015/combined-4M5S.fits has NAXIS = 3 and WCS AXES = 4, pix2world then fails expecting N x 4. Using astropy wcs methods and not sub-classing FITSWCSpix avoids the error and a reliance on the naxis. This error occurs on the old tigger-lsm and the current upstream version.

Re-implemntations have been tested against the old version of tigger-lsm and the current upstream version.


Co-authored-by: bennahugo <bennahugo@gmail.com>
@razman786
Copy link
Contributor Author

Closing this as PR #31 has fixed this.

@razman786 razman786 closed this May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants