Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-30927: Set calexp WCS to None for failed astrometric fit #573

Merged
merged 4 commits into from Sep 30, 2022
Merged

DM-30927: Set calexp WCS to None for failed astrometric fit #573

merged 4 commits into from Sep 30, 2022

Conversation

laurenam
Copy link
Contributor

No description provided.

@laurenam laurenam force-pushed the tickets/DM-30927 branch 2 times, most recently from c5ba5a8 to 6a17de4 Compare September 20, 2021 00:49
@laurenam laurenam force-pushed the tickets/DM-30927 branch 2 times, most recently from 736724f to a8ce71c Compare April 12, 2022 02:26
@laurenam laurenam force-pushed the tickets/DM-30927 branch 2 times, most recently from e2a81a8 to 24b5218 Compare April 21, 2022 17:54
@laurenam laurenam force-pushed the tickets/DM-30927 branch 4 times, most recently from d5db9e8 to 61f27db Compare July 19, 2022 20:32
@laurenam laurenam force-pushed the tickets/DM-30927 branch 2 times, most recently from 9315fef to 0cc9174 Compare July 23, 2022 01:57
erykoff
erykoff requested changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to discuss the makeWarp changes. I think it can be done much more simply, but I may be missing something.

python/lsst/pipe/tasks/calibrate.py Outdated
"photoCal, but requirePhotoCal is True.")
self.log.warning("Astrometry failed for %r, so cannot do photoCal, but requirePhotoCal "
"is False, so setting photoCalib to None and attempting to "
"proceed....", exposureIdInfo.expId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is a bit of a run-on (and has an extra . in the ellipsis...). Something like Astrometry failed for %r, so cannot do photoCal. requirePhotoCal is False, so photometric calibration will be skipped. Attempting to proceed....

python/lsst/pipe/tasks/makeWarp.py Outdated
``externalSkyCatalog`` is `None`, these are used to determine if
the calexp should be included in the warp, namely checking that it
is not `None`. If ``externalSkyCatalog`` is not `None`, this list
will be dynamically updates with the external sky WCS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> dynamically updated

python/lsst/pipe/tasks/makeWarp.py Show resolved Hide resolved
python/lsst/pipe/tasks/propagateSourceFlags.py Show resolved Hide resolved
python/lsst/pipe/tasks/selectImages.py Show resolved Hide resolved
@laurenam
Accommodate WCS to None on fail instead of raise
f58ceb7 
Instead of just raising, we now set the WCS of an exposure to None if
the single frame processing astrometry fails to find a solution and
allow the possibility for the processing to proceed (controlled via the
requireAstrometry and requirePhotoCal config parameters, more below).
As such, some accommodations must be made for downstream tasks that
require the presence of a WCS to be meaningful.  Here, on an astrometry
fail, if the config parameter requireAstrometry is True, we raise (as
in the previous behaviour).  However, if requireAstrometry is False, we
skip the match persistence (since no matches exists in this case) and we
set the photoCalib to None (again, no matching can be done if there is no
WCS to translate from pixel to sky).  All coord_ra and coord_dec columns
are set to numpy.nan (while leaving the pixel x & y values untouched).
Since, in the case of an astrometry fail, the photometric calibration
cannot proceed, in order to continue processing, the config parameter
requirePhotoCal must also be set to False.  Otherwise, the inability
to perform photometric calibration will result in a raise.

In the coaddition stage, exposures with WCS set to None cannot be
included, so they get "de-selected".

Some testing of this behavior has been added to test_wcsSelectImages.py.
Further testing happens in the ci_hsc repo (where two WCS failures have
been explicitly added).
@laurenam
Add check for WCS is None in CalexpCutoutTask
9469f2e
erykoff
erykoff reviewed Sep 21, 2022
View reviewed changes
python/lsst/pipe/tasks/makeWarp.py Outdated
calExpList = [ref.get() for ref in calExpList]
# Populate wcsList as required by new underscored version of function.
wcsList = [calexp.getWcs() for calexp in calExpList]
print("In prepareCalibratedExposures, wcsList = ", wcsList)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! Thanks for the catch!

erykoff
erykoff reviewed Sep 21, 2022
View reviewed changes
python/lsst/pipe/tasks/makeWarp.py Outdated
@@ -573,12 +638,20 @@ def prepareCalibratedExposures(self, calExpList, backgroundList=None, skyCorrLis
for index, (calexp, background, skyCorr) in enumerate(zip(calExpList,
backgroundList,
skyCorrList)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be zipping wcsList as well, instead of doing wcsList[index], for consistency/clarity if nothing else (unless there's a reason I don't see for that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really only the one instance below in
if externalSkyWcsCatalog is None and wcsList[index] is None:
that would get updated as I still need to use the wcsList[index] to assign the new external calibrations, but if you think that's still worth it, I'm happy to add it to the zip list!

@laurenam
Allow using external cals even if calexp WCS is None
ac0f96c 
This also reorders the selection of suitable calexps such that the
selection based on patch overlap is based on the final calibrated
WCS (i.e. if an external calibration is being applied, this one will
be used instead of the calibration based on single frame measurement).

This also includes the deprecation of the prepareCalibratedExposures()
function in favor of a similar version, now with a leading underscore
in the name to make it clear that this function should only be used
internally.  The deprecated version hands off to the new underscored
version for a deprecation period, but will be removed after such time.
@laurenam
Update comments and docstrings per flake8/PEP8
6f240ff
@laurenam laurenam merged commit 3906b76 into main Sep 30, 2022
@laurenam laurenam deleted the tickets/DM-30927 branch September 30, 2022 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erykoff erykoff erykoff requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@laurenam @erykoff