Repository navigation
Implement support for the -TAB algorithm in WCS - #9641
Conversation
|
This is great. LSST recently started writing files with -TAB for a small spectrograph we have. I can donate a sample file if you like? Also, wcsLib itself generates a rather complex -TAB example file. Have you tested with that file? My test files are at: https://jira.lsstcorp.org/browse/DM-21797 |
|
@timj I got your files (Thanks!) and I was able to create |
|
@timj Could someone on the LSST team test this PR as well? |
|
@MSeifert04 Since you've looked at the C wrappers before do you think you can review this one? |
|
@mcara Is it feasible to undo the purely style-related changes? It makes the PR quite complicated to review and a bit of information is lost (the |
|
This sounds like a great addition. Thank you @mcara . I'll do a full review soon, but I'm not sure if I will be able to do the review today/tomorrow. |
|
@MSeifert04 Thanks a lot for a thorough review. I hope I have addressed all your comments in the latest commit. |
| 0, /*tp_getattr*/ | ||
| 0, /*tp_setattr*/ | ||
| 0, /*tp_compare*/ | ||
| (reprfunc)PyTabprm___str__, /*tp_repr*/ |
There was a problem hiding this comment.
What is the reason for removing the repr?
If that wasn't an accidental removal we probably should also remove the function implementation of PyTabprm___str__
There was a problem hiding this comment.
I removed __repr__ because I hated the output of it when I was debugging my code. However, it is really a matter of taste and whether people prefer excessive vs succinct output. To understand (=see for yourself) what I am talking about, try:
>>> from astropy import wcs
>>> 5 * [wcs.WCS().wcs]and now please find the cdelt of the 2nd WCS in the printed output. So, for me it was easier to see how many tabprm or wtbarr structures I had in the members WCS.wcs.tab and WCS.wcs.wtb. If I wanted to see the content of one particular structure, I would do print(WCS.wcs.wtb[0]).
Maybe my above example is not appropriate for a single WCS object (and that is how it is used most of the time), but for lists of tabprm and wtbarr structures, it felt overwhelming. Maybe a more compact format can be found for __repr__ that is also more informative than a just object address.
|
@pllim Could you please be so kind as to help figuring out what is the issue with tests on Windows: https://travis-ci.org/astropy/astropy/jobs/615643939#L1980-L1990 Thank you! |
|
I can tell you it probably segfaulted but I have no desire to debug it. You can try have the CI install pytest-faulthandler and see if that gives you anything useful. Otherwise, you might need to run it locally on a Windows machine with |
3be97d3 to
04835ef
Compare
|
@MSeifert04 @nden I believe now unit tests are all passing. The reported failures are unrelated to this PR. I found another bug in WCSLIB due to which an element I will report this bug. |
|
@mcara Great 👍 I'm not sure what our policy is regarding extern libraries but I think we probably need to apply it as "patch" instead of fixing it in the extern code. Just to make it transparent what we applied and what is an extern library. |
|
I don't think we have a policy about that but in the past we've applied "patches" to a library in cextern. And by "patch" I mean make the change in the same PR which updates wcslib. @MSeifert04 do you mean apply the change to wcslib in a separate PR or something more sophisticated? |
I am actually not sure how we should patch extern libraries. But I see that my comment was misleading (probably shouldn't comment when tired 😓 ). What I meant was that the commit that fixed wcslib also changed our code-base which I think is problematic, I think the fix for an extern code-base should be restricted to the extern code-base (so that we can easily revert it with If the fix should be in a separate PR or how we should apply it (and what would be possible) is something I don't know. |
|
Now WCSLIB fix is in a separate commit. |
|
@mcara Actually do you mind applying it in a separate PR (to be merged before this one)? That way this PR contains only the |
|
@nden Allright. Will do. |
|
This looks ready to be merged. One last request @mcara, could you add an entry in the "what's new in 4.1" file? There was a recent decision to add entries there with the PR and not before a release. |
|
Yep, I think this looks okay now. Thanks @mcara ! |
|
Thanks @mcara, @MSeifert04 and everyone else who contributed! |
|
Thank you @nden and @MSeifert04 for constructive comments and bug finding work! |
8 participants
This pull request is to implement support for the
-TABalgorithm in FITS WCS as described in WCS Paper III.This PR partially addresses #2362, #5462, #7883 in the sense that
astropy.wcsnow will be able to support-TABin WCS if already defined in a HDUList object. However this PR does not provide "write" support for-TAB: the files must be created using external scripts.