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

    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 in adding support for compound datasets as well.

        Issue Links

          Activity

          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 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 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.
          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?
          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 .
          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?
          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
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Planned End:

                Development