DM-21308: Replace doApplyUberCal with doApplyExternal configs #245
Conversation
mrawls
changed the title
| retrieved from the `calexp`. When `config.doApplyExternalSkyWcs` is `True`, | ||
| the astrometric calibration is taken from `config.externalSkyWcsName` with | ||
| the `name_wcs` dataset. Otherwise, the astrometric calibration is taken | ||
| from the `calexp`. |
There was a problem hiding this comment.
Please say "processed visit image" or "processed exposure" or something other than "calexp."
| else: | ||
| photoCalib = exposure.getPhotoCalib() | ||
|
|
||
| if self.config.doApplyExternalSkyWcs: | ||
| skyWcs = dataRef.get(f"{self.config.externalSkyWcsName}_wcs") | ||
| exposure.setWcs(skyWcs) |
There was a problem hiding this comment.
This is great - the exposure already has a WCS from the calexp (heh) if this doesn't happen, so it makes sense to only setWcs if we are overriding it.
Similarly, isn't it true that the exposure already has a photoCalib from the calexp? If so, then this can be simplified even further by removing the the else above (which retrieves the photoCalib from the exposure if there's no external one) and only calls setPhotoCalib if there is a new one.
|
I haven't looked carefully at what's going on here, so apologies if I've missed the point, but... While I'm seeing review comments on changes to |
|
That's actually delightful! It turns out the comments here should apply instead to the parallel PR for pipe_tasks for DM-21308, so I will leave them for easy reference. (I looked at this PR before that one and got very confused about code duplication, so I'm thrilled it is going away) |
|
@jdswinbank It is going away, yes. And originally that other ticket (DM-20074) was a blocker for this one, but that has proved challenging so we decided to just get this one done in spite of the slight overhead of updating a mostly redundant function that is going to be removed soon. |
erykoff
force-pushed
the
tickets/DM-21308
branch
from
fe53fe5
to
425a7e6
Compare
Also update default configs.
erykoff
force-pushed
the
tickets/DM-21308
branch
from
425a7e6
to
9ab1d69
Compare
Also update default configs.