Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-23976

Move gen3 generic curated calibrations ingest code to obs_base

    XMLWordPrintable

    Details

    • 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

            Hide
            swinbank John Swinbank added a comment -

            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.

            Show
            swinbank John Swinbank added a comment - 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.
            Hide
            tjenness Tim Jenness added a comment -

            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.

            Show
            tjenness Tim Jenness added a comment - 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.
            Hide
            tjenness Tim Jenness added a comment -

            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.

            Show
            tjenness Tim Jenness added a comment - 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.
            Hide
            tjenness Tim Jenness added a comment -

            The defects read performance was fixed in DM-24055 and the DECam test now only takes 12 seconds to run instead.

            Show
            tjenness Tim Jenness added a comment - The defects read performance was fixed in DM-24055 and the DECam test now only takes 12 seconds to run instead.
            Hide
            tjenness Tim Jenness added a comment -

            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.

            Show
            tjenness Tim Jenness added a comment - 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.

              People

              Assignee:
              tjenness Tim Jenness
              Reporter:
              tjenness Tim Jenness
              Reviewers:
              John Parejko
              Watchers:
              Christopher Waters, Jim Bosch, John Parejko, John Swinbank, Simon Krughoff, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: