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

Add refcat name arg to ReferenceObjectLoader init

    XMLWordPrintable

    Details

      Description

      Eli Rykoff noted on DM-35670 that we need the name of the loaded refcat to apply colorterms, but in gen2 that was carried through as a config parameter on LoadIndexedReferenceObjectsTask. ReferenceObjectLoader doesn't currently have any knowledge of the name of the loaded refcat, but the calling Task does in the Connection name. The easiest fix for this is to add a required name kwarg to ReferenceObjectLoader.__init__(). Tasks that use a refcat loader would then pass the Connection name in runQuantum() when they create the loader.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment - - edited
            Show
            Parejkoj John Parejko added a comment - - edited Jenkins run with ci_hsc and ci_imsim: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/37209/pipeline
            Hide
            Parejkoj John Parejko added a comment - - edited

            Eli Rykoff: are you ok to review this? 8 PRs, but they're all short. I believe I caught all uses of this in the lsst org (including ones in faro that aren't actually tested!), and I didn't see any in lsst-dm. This is technically an API change since `name` is now a required arg, but I don't think anyone tries to use ReferenceObjectLoader directly, since they have to get the datarefs and htm ids first.

            Show
            Parejkoj John Parejko added a comment - - edited Eli Rykoff : are you ok to review this? 8 PRs, but they're all short. I believe I caught all uses of this in the lsst org (including ones in faro that aren't actually tested!), and I didn't see any in lsst-dm. This is technically an API change since `name` is now a required arg, but I don't think anyone tries to use ReferenceObjectLoader directly, since they have to get the datarefs and htm ids first. https://github.com/lsst/meas_algorithms/pull/290 https://github.com/lsst/pipe_tasks/pull/716 https://github.com/lsst/analysis_tools/pull/27 https://github.com/lsst/faro/pull/139 https://github.com/lsst/fgcmcal/pull/90 https://github.com/lsst/jointcal/pull/229 https://github.com/lsst/analysis_drp/pull/50 https://github.com/lsst/atmospec/pull/24 https://github.com/lsst/ap_pipe/pull/124 https://github.com/lsst/drp_pipe/pull/38 https://github.com/lsst/obs_decam/pull/231 https://github.com/lsst/obs_lsst/pull/420 https://github.com/lsst/obs_subaru/pull/433
            Hide
            erykoff Eli Rykoff added a comment -

            There are some docstring and usage comments on the meas_algorithms and pipe_tasks PRs. Otherwise looks good.

            Show
            erykoff Eli Rykoff added a comment - There are some docstring and usage comments on the meas_algorithms and pipe_tasks PRs. Otherwise looks good.
            Hide
            Parejkoj John Parejko added a comment - - edited

            Sorry for the trouble, Eli Rykoff: after your review, I reworked some things and added a few more PRs to remove the `ref_dataset_name` usage everywhere I could, and replace it with this new `name` parameter if necessary. Please take another look, and note that there are several more PRs (all linked above).

            Show
            Parejkoj John Parejko added a comment - - edited Sorry for the trouble, Eli Rykoff : after your review, I reworked some things and added a few more PRs to remove the `ref_dataset_name` usage everywhere I could, and replace it with this new `name` parameter if necessary. Please take another look, and note that there are several more PRs (all linked above).
            Hide
            erykoff Eli Rykoff added a comment -

            This looks good. Thanks for going through and cleaning up all the deprecated usage of ref_dataset_name!

            Show
            erykoff Eli Rykoff added a comment - This looks good. Thanks for going through and cleaning up all the deprecated usage of ref_dataset_name !
            Hide
            Parejkoj John Parejko added a comment -

            I've merged everything except the atmospec PR, while I wait for feedback from Merlin Fisher-Levine; I think there's an already-broken gen2 refcat usage in processStar.py, but the tests don't run it.

            Show
            Parejkoj John Parejko added a comment - I've merged everything except the atmospec PR, while I wait for feedback from Merlin Fisher-Levine ; I think there's an already-broken gen2 refcat usage in processStar.py, but the tests don't run it.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            As I said on the PR, look like atmospec was already broken, so go ahead and merge, and we'll work on fixing it up later.

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - As I said on the PR, look like atmospec was already broken, so go ahead and merge, and we'll work on fixing it up later.

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              Eli Rykoff
              Watchers:
              Eli Rykoff, Ian Sullivan, Jim Bosch, John Parejko, Merlin Fisher-Levine, Nate Lust
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.