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.
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
DM-30927: Set calexp WCS to None for failed astrometric fit #147
Conversation
b8397f1
to
fc2783b
Compare
fc2783b
to
5e93b90
Compare
d9b92e9
to
97600f9
Compare
97600f9
to
d141a7b
Compare
tests/test_astrometryTask.py
Outdated
results = solver.run( | ||
sourceCat=sourceCat, | ||
exposure=self.exposure, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also possible to test if we log a warning as expected? (I actually don't know how to do that...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. with self.assertLogs()
(if it's a C++ lsst.log log message you have to use an additional bit of code to redirect the C++ logging to python logging)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to look this up, but I think I've got the proper check in à la:
with self.assertLogs(level=logging.WARNING) as cm:
results = solver.run(
sourceCat=sourceCat,
exposure=self.exposure,
)
logOutput = ";".join(cm.output)
self.assertIn("WCS fit failed.", logOutput)
self.assertIn("Setting exposure's WCS to None and coord_ra & coord_dec cols in sourceCat to nan.",
logOutput)
md['SFM_ASTROM_OFFSET_STD'] = tryMatchDist.distStdDev.asArcseconds() | ||
if fitFailed: | ||
md['SFM_ASTROM_OFFSET_MEAN'] = np.nan | ||
md['SFM_ASTROM_OFFSET_STD'] = np.nan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually uncertain about this one. We do have a measurement of the OFFSET_MEAN and OFFSET_STD, it's just that they failed our threshold, right? I think having these data recorded (how bad was it?) is valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that too. I landed on thinking it may be too confusing for the uninitiated to come across a calexp
that has WCS set to None
, yet has astrometric metadata associated with it, so opted for making the official associated metadata NaN
s. The information is not entirely lost as these numbers do get printed to the logs, so, e.g.
In [34]: root = "/sdf/data/rubin/u/laurenma/LSST/ci_hsc_gen3/DATA"
In [35]: collection = "HSC/runs/ci_hsc"
In [36]: camera = "HSC"
In [37]: butler = Butler(root, instrument=camera, collections=collection)
In [38]: cal_log = butler.get("calibrate_log", detector=0, visit=903344)
In [39]: for logLine in cal_log:
...: if "Matched and fit WCS" in logLine.message or "Assigning as a fit failure" in logLine.message:
...: print(logLine.message)
...:
Matched and fit WCS in 3 iterations; found 181 matches with mean and scatter = 0.028 +- 0.018 arcsec
Assigning as a fit failure: mean on-sky distance = 0.028 arcsec > 0.025 (maxMeanDistanceArcsec)
I do recognize the pain in parsing individual logs, so I'm happy to go with whatever Sir Calibration thinks is most useful in the grand scheme of things!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments on recording the stats from the bad fit.
self.assertTrue(results.matches is None) | ||
self.assertTrue(np.all(np.isnan(sourceCat["coord_ra"]))) | ||
self.assertTrue(np.all(np.isnan(sourceCat["coord_dec"]))) | ||
self.assertTrue(self.exposure.getWcs() is None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be testing the metadata (though maybe not to check if they're nans, per my previous comment.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I can check the metadata you're looking for here. I did add checks for:
self.assertTrue(results.scatterOnSky is None)
self.assertTrue(results.matches is None)
and I have a placeholder check for the metadata in ci_hsc_gen3
as:
def testMetadataForFailedAstrom(self):
"""Test that the metadata for a failed astrometic fit is set properly.
Wait until Eli weighs in on what he wants this to be before committing.
"""
calexpMetadata = self.butler.get("calexp.metadata", self.calexpMinimalDataId)
self.assertTrue(np.isnan(calexpMetadata["SFM_ASTROM_OFFSET_MEAN"]))
self.assertTrue(np.isnan(calexpMetadata["SFM_ASTROM_OFFSET_STD"]))
d141a7b
to
f251dd7
Compare
This allows the task to continue and produce outputs rather than raising and aborting. Downstream tasks can decide how they wish to handle "calexp" and "src" datasets for which there is no WCS solution.
f251dd7
to
2854892
Compare
No description provided.