Repository navigation
Upgrade WCSLIB to 5.x -- without using it for SIP - #3878
Conversation
|
@nden: You may want to try this instead. I think it's a lot lower risk than letting wcslib handle the SIP. |
|
As far as I'm concerned, this is fine to merge once it's been reviewed. |
|
Wow, that sounds like it was really annoying to deal with. I wonder if it would be at all useful, for now, to have a flag somewhere (maybe even just hidden--not as part of the public API) to allow WCSLIB to handle SIP corrections, for testing purposes if nothing else. |
|
@mdboom HST tests passed. |
|
@embray: I think once wcslib's SIP support does everything we need (it's mainly just missing things like copying/pickling and round-tripping at this point) then it might make sense to have an option to use it or not. But I don't think there's much point for an option now since it's known to be broken. |
There was a problem hiding this comment.
@mdboom I read how TPV is defined here http://iraf.noao.edu/projects/ccdmosaic/tpv.html and just for my understanding - I'm wondering why this is a valid test, given that the order of operations is different (in the WCS paper language one is prior, the other sequent).
It is possible to convert from one type to the other. Assuming wcslib converts them internally, were the TPV coefficients in this test created from the SIP coefficients and so basically this tests the agreement between astropy.wcs.sip and wcslib sip? Or am I misunderstanding it?
There was a problem hiding this comment.
This is comparing two files from the same data source -- one with the polynomial in TPV form and one in SIP form. It is intended that they have the same transformation, even though the exact values in the coefficients are different due to the differences in conventions in TPV and SIP. It's testing agreement between astropy's SIP and wcslib's TPV on different headers that are meant to match. See @eteq's report here: #3845 (comment)
There was a problem hiding this comment.
Hit send too soon: Also, my understanding is TPV can be sequent or prior depending on how it's defined, and in the case of this file it is prior, as SIP always is.
These changes are intended to be backward compatible, so we can still build against wcslib 4.x
183cbdc to
be09b26
Compare
|
👍 for merging from me. @mdboom Do you want to restart Travis or is the failure a real problem? |
|
I've restarted the failed Travis job... looks like an unrelated spurious error. |
Upgrade WCSLIB to 5.x -- without using it for SIP
|
🎉 |
|
👏 |
This is a zero-regression workaround version of #3845 that doesn't shift to using wcslib for handling SIP. Instead, it parses the SIP keywords, as before, except it deletes the SIP keywords from the header before passing them along to wcslib, so everything happens as it did back in the SIP-unaware wcslib days. This buys us some time (and allows packagers to upgrade to wcslib 5.6 or later if they choose without breaking astropy) before wcslib is truly ready to be a replacement for all of the SIP functionality that astropy already has.