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- 26629: switch to calibration collections instead of the calibration_label dimension #303
Conversation
628e67c
to
0d3e5a3
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.
Looks good.
default name may not work well for long-lived repositories unless | ||
one or more ``suffixes`` are also provided (and changed every time | ||
curated calibrations are ingested). | ||
suffixes : `Sequence` [ `str` ], optional |
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.
Where should the obs_x_data version come into this? As a default suffix specifically for the standard text calibrations?
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.
That's the sort of thing I was thinking, but this gets into some of the bigger questions about how obs_x_data will be used in the certification workflow in detail, and right now I just want to punt that to @czwa and DMTN-148; I think it's sufficient for now that we have a place in the naming conventions to put whatever extra identifier comes out out that.
@@ -425,30 +425,15 @@ def findDatasets(self): | |||
self.task.log.info("Adding special datasets in repo %s.", self.root) | |||
for dataset in self.iterDatasets(): | |||
assert len(dataset.refs) == 1 | |||
self._fileDatasets[dataset.refs[0].datasetType].append(dataset) | |||
self._fileDatasets[dataset.refs[0].datasetType][None].append(dataset) |
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.
Maybe add a short comment here explaining that the None is (I think) the calib date?
for datasetTypeName in self.curatedCalibrationDatasetTypes: | ||
with self.subTest(dtype=datasetTypeName, dataId=dataId): |
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.
Probably should keep the subTest here so we can tell which dataset type is broken.
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.
The dataset type name is already in the assert message below.
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.
although subTest runs the full loop whereas a failure in the first item in the loop stops the entire test.
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, which I usually hate, but it seems safe here. I'll put it back in.
This lets us move some datasets to their own RUN collection during conversion rather than forcing us to put them in the main RUN for that Gen2 rerun. That's useful in at least ci_hsc, where we have to put the brightObjectMasks in the rerun in Gen2 because that's where the skymap is defined, but we'd rather put them in a top-level collection in Gen3.
0d3e5a3
to
aff2274
Compare
No description provided.