Upgrade wcslib to 5.x (using wcslib for SIP) - #3845

Since this now superceded #3751: Is this one planned for v1.1.0?

@olebole: Tentatively planned for 1.1, yes. That depends on whether the remaining functionality gets into wcslib by then or not.

It seems there's still an issue in the inverse transform of SIP here. Haven't got to the bottom of it yet...

@mdboom - I'm still not able to get DECam images (It uses TPV, and come from a major NOAO facility instrument) to work correctly with this. Steps to reproduce:

1. Go to http://www.portal-nvo.noao.edu/search/query and select "CTIO 4m + DECam imager" under Telescope&Instrument, and check "calibrated images" for "Mosaic, NEWFIRM, and DECam". If you want exactly the file I tested on, you'll also want to choose Program number="2012B-0001", but I don't think that actually matters
2. Hit "search", and then find the first result the search returns that has "Product"="image". Hit "retreive" on that row and it will download a DECam image.
3. You should see this with that image:

>>> f = fits.open('c4d_130831_235507_ooi_g_d3.fits.fz')
>>> w = wcs.WCS(f[1].header)
>>> ra, dec = w.all_pix2world([0,10, 15],[0,10, 20],0)
>>> ra
array([ 301.2065433,  301.2065433,  301.2065433])
>>> dec
array([-50.93399553, -50.93399553, -50.93399553])

which is clearly wrong: they should be different ra/dec.
4. But also, I see this:

>>> ra, dec = w.wcs_pix2world([0,10, 15],[0,10, 20],0)
>>> ra
array([ 300.49796386,  300.49908106,  300.50020568])
>>> dec
array([-49.95258804, -49.95332569, -49.95369924])

which looks like it is correct - and the only difference is wcs_pix2world instead of all_pix2world.

So that suggests the problem is in the layers after wcslib - that's weird to me because I thought the point of this PR was that wcslib now supports TPV so nothing else should be needed (but I admit the internals of this package aren't too familiar to me).

@eteq: Thanks for the info. It seems the issue is that when set is called multiple times on a WCS object (as it is as a matter of course as values are updated etc.) the distortion coefficient values are erased. And when you have coefficients of all zeros, the entire linear transformation disappears, which is why you're seeing all the same values in that case. I'll take this up with the wcslib author.

I was planning on leaving it open -- eventually we should be using this approach, but there are still some important missing pieces...

@nden - I wonder whether we should close this, or there are still relevant pieces in this PR that need to be merged?

Closing this as a PR and open an issue to remind ourselves to the SIP issue.

@bsipocz My impression is that a lot of the work needed for #6030 is actually in this PR. Or am I wrong?

@nden - That is very possible, I'm not familiar enough with wcs to see into the details. However this PR was opened before github allowed maintainers to overtake abandoned PRs and had tons of conflicts. Thus even if someone takes the work over from here, they need to open a new PR.

Keep this open as a reference.

@nden - As a reference I would suggest to open an issue instead, as this PR was opened before github allowed maintainers to take over (thus any work on this needs to be done in a new PR anywy). I've tried to rebase this without the cextern/wcslib updates, but ended up with horrible test failures, so this definitely need to be done by someone knowing the wcs module.

@bsipocz I don't expect this work to be done by someone who does not understand astropy.wcs, at least not without some help. I keep it open mostly for my own reference. Unfortunately working on this is not high on my priorities list. I could instead create a branch in my own repo with Mike's work and then we can close but no one who understands astropy.wcs will be able to see it. I can't think why an issue would be better. What kind of issue do you have in mind?

@nden - I just had the "close stale PRs" e-mail of astropy-dev in mind, that we could aim to keep issues open as reminders rather than PRs that can't be finished in their current form. However it's fine to keep this open if you find it more useful as a reminder than either #6030 or #6371. (I don't think that the auto close will get to this any time soon, though).

Hi there @mdboom 👋 - 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 labelled 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 😃.

I see this is a work in progress pull request. I'll report back on the checks once the PR is ready for review.

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

Hi humans 👋 - this issue was labeled as Close? approximately 6 months ago. So..... any news? 🗞️

If you think this issue should not be closed, a maintainer should remove the Close? label - otherwise, I'm just gonna have to close this issue in a month. Your time starts now! Tick tock 🕙

If you believe I commented on this issue incorrectly, please report this here

Hi humans 👋 - this pull request hasn't had any new commits for approximately 2 years. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please close this and open an issue instead to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If you believe I commented on this issue incorrectly, please report this here.

⏰ Time's up! ⏰

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.