Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-204

Move all config and tract/patch dataset definitions to base CameraMapper

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      We now have the option to define mapper datasets that are the same for all cameras in a common location (the config files for the base class CameraMapper). I propose that we go ahead and do this for all common datasets - essentially anything that doesn't depend on the keys of the raw data ID - now. This will massively reduce code duplication in the mapper files, and reduce our dependence on paf files (since we'll be moving to the new YAML-ish format).

      There are two possible hangups:

      • We probably need to resolve DM-6858 first (we currently compare the list of centrally-defined datasets against a list hard-coded into the test).
      • This will change the location of some data products for at least some cameras. I think this is a good thing in the long run, as there's no reason for different cameras to put common datasets in different locations, but this opinion may not be unanimous, and it may cause some backwards compatibility problems. If these are important, I suggest we define additional backwards compability mappers that do not move these datasets for a transitional period. The good news is that a preliminary look suggests that most of our mappers already define things consistently; obs_sdss may be the only one that uses different conventions for coadd datasets.

      I do not have anyone already signed up to do this work (I'm hoping to get by on the kindness of TCAMs), but this really shouldn't be more than a couple of days - there are a lot of lines to move and reformat, but it's dead simple.

      I do believe doing this sooner rather than later (and in particular not waiting on the dynamic dataset definition feature) will make Science Pipelines developers' lives easier, and I think it may be helpful to Nate Pease [X] in adding support for compound datasets as well.

        Attachments

          Issue Links

            Activity

            No builds found.
            jbosch Jim Bosch created issue -
            Hide
            rhl Robert Lupton added a comment -

            This looks good, but I don't know (and can't see from DM-6858) how we'll handle conflicts. Is there something equivalent to virtual (and final, and delete) from C++11 to handle this sort of `inheritance'?

            Show
            rhl Robert Lupton added a comment - This looks good, but I don't know (and can't see from DM-6858 ) how we'll handle conflicts. Is there something equivalent to virtual (and final, and delete) from C++11 to handle this sort of `inheritance'?
            Hide
            rowen Russell Owen added a comment -

            I am in favor of standardizing what dataset types we can, so I like this proposal. Robert Lupton raises a good question about conflicts. For the record, I'd rather transition now and put up with lack of backward compatibility, as it will only get harder later. We can reprocess SDSS data if need be.

            Show
            rowen Russell Owen added a comment - I am in favor of standardizing what dataset types we can, so I like this proposal. Robert Lupton raises a good question about conflicts. For the record, I'd rather transition now and put up with lack of backward compatibility, as it will only get harder later. We can reprocess SDSS data if need be.
            Hide
            jbosch Jim Bosch added a comment -

            I had assumed all dataset definitions were "virtual" (the most derived mapper's definition wins), and that there was no way to declare them final or delete them. I think that meets our needs, but I'd love to get confirmation from Nate Pease [X] and/or Kian-Tat Lim that this is indeed how it works.

            Show
            jbosch Jim Bosch added a comment - I had assumed all dataset definitions were "virtual" (the most derived mapper's definition wins), and that there was no way to declare them final or delete them. I think that meets our needs, but I'd love to get confirmation from Nate Pease [X] and/or Kian-Tat Lim that this is indeed how it works.
            Hide
            price Paul Price added a comment -

            I was thinking to do this as part of the hack session in August, so thanks for RFCing it.

            Regarding the compatibility problems, I believe what's defined in CameraMapper can be overridden by the subclass if desired. So we can always add everything in daf_butlerUtils, and then remove the definitions in the obs packages on a case by case basis.

            Show
            price Paul Price added a comment - I was thinking to do this as part of the hack session in August, so thanks for RFCing it. Regarding the compatibility problems, I believe what's defined in CameraMapper can be overridden by the subclass if desired. So we can always add everything in daf_butlerUtils, and then remove the definitions in the obs packages on a case by case basis.
            Hide
            jbosch Jim Bosch added a comment -

            Implementation issue is DM-7049, which will only make strictly backwards-compatible changes. It will also spawn new per-camera issues for non-backwards-compatible standardization.

            Show
            jbosch Jim Bosch added a comment - Implementation issue is DM-7049 , which will only make strictly backwards-compatible changes. It will also spawn new per-camera issues for non-backwards-compatible standardization.
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Link This issue relates to DM-7049 [ DM-7049 ]
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            tjenness Tim Jenness made changes -
            Link This issue is triggering DM-7049 [ DM-7049 ]
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-7049 [ DM-7049 ]
            Hide
            tjenness Tim Jenness added a comment -

            Now that DM-7049 has been completed, is this RFC now complete and ready to be marked Implemented?

            Show
            tjenness Tim Jenness added a comment - Now that DM-7049 has been completed, is this RFC now complete and ready to be marked Implemented?
            tjenness Tim Jenness made changes -
            Link This issue is blocked by DM-6858 [ DM-6858 ]
            Hide
            jbosch Jim Bosch added a comment -

            Added another implementation ticket, DM-7677.

            Show
            jbosch Jim Bosch added a comment - Added another implementation ticket, DM-7677 .
            jbosch Jim Bosch made changes -
            Link This issue is triggering DM-7677 [ DM-7677 ]
            swinbank John Swinbank made changes -
            Remote Link This issue links to "Page (Confluence)" [ 14407 ]
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch is this RFC now implemented?

            Show
            tjenness Tim Jenness added a comment - Jim Bosch is this RFC now implemented?
            jbosch Jim Bosch made changes -
            Status Adopted [ 10806 ] Implemented [ 11105 ]
            Hide
            jbosch Jim Bosch added a comment -

            Yes, marked as such.

            Show
            jbosch Jim Bosch added a comment - Yes, marked as such.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Watchers:
              Jim Bosch, John Swinbank, Paul Price, Robert Lupton, Russell Owen, Tim Jenness, Xiuqin Wu [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins Builds

                  No builds found.