Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: obs_base, obs_decam, obs_lsst, obs_subaru
-
Labels:
-
Story Points:2
-
Team:Architecture
-
Urgent?:No
Description
In DM-23778 I updated writeCuratedCalibrations to support QE curves as well as defects. This is generic code that uses the pipe_tasks read_all function to read standardized calibration data from obs_x_data packages. Since by definition these calibrations are standardized we should move this code to obs_base and ensure that all gen3 instruments call it if they have defined an _data package. The relevant dataset types and curated calibrations would only be created/ingested if they exist in the _data package (which by definition means they are standardized). The instrument subclasses should only be required to define their non-standard curated calibrations.
Attachments
Issue Links
Activity
I'm going to do this refactoring because at the moment we have more or less the same code in multiple obs packages and if/when we decide that we will do this differently then it's still true that defects will be handled the same in all the obs packages.
John Parejko This is mostly moving code around so it shouldn't take much to review. I'm a bit concerned that my method names are too long so feel free to suggest improvements there. I've stuck with writeCuratedCalibrations as the general concept so that we minimize changes and focus solely on the code reuse issue.
The biggest "problem" might be that it takes 5 minutes on my laptop to ingest all the defects files so that new test takes a while.
The defects read performance was fixed in DM-24055 and the DECam test now only takes 12 seconds to run instead.
I made a mistake on this ticket in that I dealt with what I thought was the review on GitHub and merged the branches before I realized that John Parejko had not formally approved the work in Jira.
The two issues that I thought I resolved were:
- Not counting datasets as part of testing. I didn't want to do this because I don't want to be in a situation where every time a new calibration is added to obs_x_data the person adding the calibrations has to also make a one line change to obs_x. There is a difference between testdata packages and the more dynamic obs data packages.
- Using a new policyName attribute for finding configs. We can't use .lower because lsstCam has mixed case. If we want to always use lower case or, more disruptive, always use the case sensitive instrument name then we will need an RFC for that.
John Parejko let me know what you feel the current situation is. I can create new supplemental branches if needed.
So I don't think we should block progress being made in terms of deployed functionality pending the availability of documentation..
But I also think we need to get this written down so that we can discuss it and agree if it's what we actually want to do. Even if Tim and Simon and Chris all agree it's a model they like, then we still need to give others the chance to object.