-
Notifications
You must be signed in to change notification settings - Fork 0
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-43077: Convert all tasks to use CalibrateImageTask outputs #119
base: main
Are you sure you want to change the base?
Conversation
8171aac
to
ef6b6d1
Compare
ef6b6d1
to
2ff3535
Compare
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.
Seems fine as it's mostly just renaming (which I have no opinion on), assuming it passes. See individual comments.
@@ -55,14 +55,14 @@ HIPS_QGRAPH_FILE=$(mktemp)_hips.qgraph | |||
trap 'rm -f $QGRAPH_FILE $INJECTION_QGRAPH_FILE $FARO_QGRAPH_FILE $RESOURCE_USAGE_QGRAPH_FILE \ | |||
$HIPS_QGRAPH_FILE' EXIT | |||
|
|||
# DM-46272: Change maxMeanDistanceArcsec to something smaller, so that it fails; | |||
# even better, move those settings into DRP-ci_hsc.yaml! |
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 was just going to ask, is there any good reason for these not to be in DRP-ci_hsc.yaml
?
@@ -55,14 +55,14 @@ HIPS_QGRAPH_FILE=$(mktemp)_hips.qgraph | |||
trap 'rm -f $QGRAPH_FILE $INJECTION_QGRAPH_FILE $FARO_QGRAPH_FILE $RESOURCE_USAGE_QGRAPH_FILE \ | |||
$HIPS_QGRAPH_FILE' EXIT | |||
|
|||
# DM-46272: Change maxMeanDistanceArcsec to something smaller, so that it fails; |
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.
Add TODO:
?
# set. This list is sensitive to the astrometry algorithms and dataset | ||
# under consideration, so may require updating if either of those change | ||
# in the context of this repository. | ||
# DM-46272: not forcing these failures until we can handle partial outputs; |
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.
Add TODO:
?
@@ -29,48 +29,46 @@ | |||
from lsst.utils import getPackageDir | |||
|
|||
|
|||
# DM-46272: not forcing these failures until we can handle partial outputs; |
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.
Add TODO:
?
instrument="HSC", detector=self.detector, visit=self.visit, | ||
universe=self.butler.dimensions, | ||
) | ||
|
||
def tearDown(self): |
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.
Why is this being removed? I understand that it's not really necessary to delete every primitive but deleting more heavyweight objects like self.butler
seems justifiable rather than waiting for the GC to collect it whenever.
@@ -44,14 +44,6 @@ def setUp(self): | |||
universe=self.butler.dimensions, | |||
) | |||
|
|||
def tearDown(self): |
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.
Same as above.
class TestCalibrateOutputs(lsst.utils.tests.TestCase, MockCheckMixin): | ||
"""Test the output data products of calibrate task make sense | ||
class TestReprocessVisitImageOutputs(lsst.utils.tests.TestCase, MockCheckMixin): | ||
"""Test the output data products of reprocessVisitImage task make sense. |
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.
Test that...
2ff3535
to
c84a0e6
Compare
calibrateImage needs a tighter constraint here to produce a failure, and it's only in one detector/visit.
Until we've fully figured out how to handle partial outputs in DRP tasks downstream of calibrateImage, we can't use this to test failed astrometry.
c84a0e6
to
f2e5f80
Compare
No description provided.