Repository navigation
Upgrade wcslib to 5.7 - #3905
Conversation
mdboom
added
wcs
Affects-dev
There was a problem hiding this comment.
Maybe have this instead be something like "only 5.8 and later (or the verion packaged with astropy)"? I.e., make it clear that a possible "solution" is to use the builtin one if for whatever reason it's difficult to upgrade wcslib
|
One other thought: maybe you should actually add a test using only the headers from an image HDU copied from a DECAM image (a la what I was checking in #3904)? That seems like a decent way to do a regression test without the full image. |
I already have a test using a |
|
@mdboom - oops, missed that. So yes, that's perfect then. |
|
Seems good to go then. |
There was a problem hiding this comment.
@mdboom Is there a reason this line was removed? If empty lines in the header don't get removed, calling to_header creates fits cards like keyword=" " , value = None. This came up just now because wcslib 5.8 adds an empty line and a comment line. Here's what I mean:
w.to_header()
Out[13]:
WCSAXES = 2 / Number of coordinate axes
CRPIX1 = 2048.0 / Pixel coordinate of reference point
CRPIX2 = 1024.0 / Pixel coordinate of reference point
PC1_1 = 1.29055156974E-05 / Coordinate transformation matrix element
PC1_2 = 5.95250078647E-06 / Coordinate transformation matrix element
PC2_1 = 5.02263819607E-06 / Coordinate transformation matrix element
PC2_2 = -1.26448441522E-05 / Coordinate transformation matrix element
CDELT1 = 1.0 / [deg] Coordinate increment at reference point
CDELT2 = 1.0 / [deg] Coordinate increment at reference point
CUNIT1 = 'deg' / Units of coordinate increment and value
CUNIT2 = 'deg' / Units of coordinate increment and value
CTYPE1 = 'RA---TAN-SIP' / TAN (gnomonic) projection + SIP distortions
CTYPE2 = 'DEC--TAN-SIP' / TAN (gnomonic) projection + SIP distortions
CRVAL1 = 5.63056810618 / [deg] Coordinate value at reference point
CRVAL2 = -72.0545718428 / [deg] Coordinate value at reference point
LONPOLE = 180.0 / [deg] Native longitude of celestial pole
LATPOLE = -72.0545718428 / [deg] Native latitude of celestial pole
WCSNAME = 'IDC_postsm4' / Coordinate system title
RADESYS = 'FK5' / Equatorial coordinate system
EQUINOX = 2000.0 / [yr] Equinox of equatorial coordinates
COMMENT WCS header keyrecords produced by WCSLIB 5.8
I have code that loops over WCS keywords and changes them. I'll change my code to look for empty lines and comments but just wondering if the previous behaviour is not preferable,
There was a problem hiding this comment.
I think that line may just have been removed in error -- there was lots of juggling how the header is parsed here so we can edit it and pass the edited version (with SIP removed for example) to wcslib.
But I'm not sure how the strip would help here -- it wouldn't remove any blank FITS lines unless they were at the end.
There was a problem hiding this comment.
Yes, you are right. However the absence of this line, and the addition of a new line and a comment have the same effect, in the sense that they create keywords which are not in the WCS standard and I expect WCS.to_header to return WCS keywords only. Yeah, I agree it returns a valid header but I wish this kind of behaviour change was handled through the relax keyword or not introduced at all. It just adds noise.
There was a problem hiding this comment.
We could modify to_header to remove these lines...
There was a problem hiding this comment.
👍 Do you want me to make a PR or do you prefer to make it? It's either removing these lines or I have to make numerous changes in stwcs.
There was a problem hiding this comment.
👍 This change is unexpectedly hard to handle.
There was a problem hiding this comment.
I almost have a PR -- stay tuned...
This contains the "official" fix for the iteration bug in #3891, as well as a fix for the copying of distortion parameters in wcsprm objects in #3904.
In addition to vanilla 5.7, this includes a small fix for outputting FITS headers containing TPV distortions that in all likelihood will make it into wcslib 5.8. I've therefore designated the version 5.8.pre. (As well as a test for the issue by round-tripping a TPV WCS).
As per a discussion with @eteq and @astrofrog, this also adds a check that will raise an
ImportErrorif the version of wcslib is one of the known broken ones in the 5.x series (currently anything prior to to-be-released 5.8). Unfortunately, there's no easy way to do this at build time -- it happens on the first import of a built astropy (which obviously includes running the tests). There was no version specifier in versions prior to 5.x, so we just act as if all 4.x versions are fine (which isn't quite true).This also adds a test for the issue in #3904.