DM-19857: Update for new defects ingestion. #68
Conversation
This simplifies how defects are ingested because they now come from a known location rather than an ad hoc one.
python/lsst/ap/verify/dataset.py
Outdated
| @@ -142,7 +142,7 @@ def calibLocation(self): | |||
| def defectLocation(self): | |||
| """The directory containing defect files (`str`, read-only). | |||
| """ | |||
| return self.calibLocation | |||
| return os.path.join(getPackageDir('obs_decam_data'), 'decam', 'defects') | |||
There was a problem hiding this comment.
Please do not hard-code dependencies on specific instruments into ap_verify. If defects are no longer part of the dataset format, then removing the defectLocation member would be more appropriate.
There was a problem hiding this comment.
Removing defectLocation required more surgery than I had hoped to do. Do you think you could have a look and suggest how best to do that?
But I agree that what I did was not right.
There was a problem hiding this comment.
It looks like, outside of test code, the only place where defectLocation gets used is in your call to _doIngestDefects. So it might make more sense to make that a config field in DatasetIngestConfig, which will at least be consistent with how the inputs to IngestCalibsTask are handled.
The defect location could be configured in the dataset-specific config files, but it could also be put in an obs package config override file (already supported as datasetIngest.py). I think you need to modify those files anyway because you removed config fields.
AFAIK the only obs package overrides for DatasetIngestTask are in obs_subaru and obs_decam. There's one in obs_test, but it doesn't configure defects at all.
There was a problem hiding this comment.
@kfindeisen I've implemented your suggestions. I've updated obs_test and obs_decam though on a different ticket since they have to be merged together anyway: tickets/DM-18739 and tickets/DM-19730 respectively. I didn't see anything to change in obs_subaru. I hope this addresses your concerns.
@mrawls if you are happy with things as they stand, will you approve this please, or let me know what else I need to do?
| self.log.info("No defects to ingest, skipping...") | ||
| self.log.info("Ingesting defects...") | ||
| self._doIngestDefects(workspace.dataRepo, workspace.calibRepo, dataset.defectLocation) | ||
| self.log.info("Defects are now ingested in {0}".format(workspace.calibRepo)) |
There was a problem hiding this comment.
Does there need to be a way to disable defect ingestion for instruments that don't use it (e.g., HSC), or is that something IngestDefectsTask takes care of?
There was a problem hiding this comment.
It looks like the changes in obs_decam result in ingested defects always landing in obs_decam_data. Do we even need to deal with defects in ap_verify at all anymore?
There was a problem hiding this comment.
They do not land in obs_decam_data. They land in workspace.calibRepo.
They will need to be ingested in order to use them.
| self.log.info("No defects to ingest, skipping...") | ||
| self.log.info("Ingesting defects...") | ||
| self._doIngestDefects(workspace.dataRepo, workspace.calibRepo, dataset.defectLocation) | ||
| self.log.info("Defects are now ingested in {0}".format(workspace.calibRepo)) |
There was a problem hiding this comment.
It looks like the changes in obs_decam result in ingested defects always landing in obs_decam_data. Do we even need to deal with defects in ap_verify at all anymore?
|
As I say above, they are not ingested into a calib repo in |
|
There's a reference to defect files in |
In light of DM-18739, we needed to update how this package manages that.