DM-10765: Replace existing WCS classes with SkyWcs #23
Conversation
r-owen
force-pushed
the
tickets/DM-10765
branch
from
bcc3d27
to
5abddf0
Compare
r-owen
force-pushed
the
tickets/DM-10765
branch
from
5abddf0
to
6096612
Compare
r-owen
force-pushed
the
tickets/DM-10765
branch
from
6096612
to
9bf2ec6
Compare
tests/testPsfexPsf.py
Outdated
| @@ -9,7 +9,7 @@ | |||
| # | |||
| # This program is free software: you can redistribute it and/or modify | |||
| # it under the terms of the GNU General Public License as published by | |||
| # the Free Software Foundation, either version 3 of the License, or | |||
| # the Free Software Foundation, either vers[1.0ion 3 of the License, or | |||
| wcs->crder[i] = wcsPrm->crder[i]; | ||
| wcs->csyer[i] = wcsPrm->csyer[i]; | ||
| wcs->crval[i] = wcsPrm->crval[i]; | ||
| auto ifits = i + 1; |
There was a problem hiding this comment.
I'm not sure what all these wcs parameters mean here and below. I guess they are safe to be set to zero?
There was a problem hiding this comment.
I think so. I could not find any psfex documentation for this structure, so I set what fields I could based on the wcslib wcsprm docs. crdr and csyer are uncertainties, which we have no way of knowing, so I thought 0 was reasonable.
| wcs->lat = wcsPrm->lat; | ||
| wcs->lng = wcsPrm->lng; | ||
| wcs->equinox = wcsPrm->equinox; | ||
| wcs->lng = 0; |
There was a problem hiding this comment.
Again, I'm unsure about these parameters, but you are not setting some and fixing others.
There was a problem hiding this comment.
I think the only fields I am not setting are lonpole and latpole. The defaults should suffice and they are a nuisance to determine because they depend on the sky projection used. I suppose it might be worth seeing if they show up in the metadata and setting them if so. But it is highly unlikely we will ever use anything except the default, and the rules for setting them are a bit complex (since some PV terms override the LONPOLE and LATPOLE). As an aside: the wcs struct field wcsstruct has lonpole misspelled as longpole, but I assume that's in the psfex code and nothing we would be inclined to try to change.
|
Are these (reasonable but speculative) changes tested, in the sense that psfex still works?
|
|
I'm, too, am uneasy about the handling of the wcsstruct fields as well. The fact that psfex documentation says nothing about wcsstruct is alarming. As far as it working: all I can say is the unit tests pass. |
|
I think we need a higher standard than the unit tests. @PaulPrice will know, but ci_hsc is probably sufficient
|
|
I'm sure @r-owen will run everything through ci_hsc before he merges, if he hasn't already. |
r-owen
force-pushed
the
tickets/DM-10765
branch
from
9bf2ec6
to
4e6553f
Compare
|
Yes, of course I will run ci_hsc, but I have not done so yet. |
No description provided.