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

Need values for missing dataset template in various packages.

    Details

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

      Description

      Per DM-8432, when a datasetType template parameter is declared but not defined by the policy in obs_base (in obs_base policy files these are written like template: ''), the template must be defined in the packages that have mappers that derive from CameraMapper. This would allow the mapper initialization to fail early when the policy is not completely defined (at mapper init, instead of failing only when Butler client code attempts to use the datsetType).

      This has been implemented, but there are packages that don't define the template in some datasetTypes. The package, mapper, datasetType, and missing key (only template in this case) are listed below.

      Can package experts please provide the correct value for these?

      obs_subaru

      HscMapper exposures.dcrCoadd_directWarp, missing key: template
      HscMapper exposures.deepDiff_differenceExp, missing key: template
      HscMapper exposures.deepDiff_matchedExp, missing key: template
      HscMapper datasets.deepDiff_diaSrc, missing key: template
      HscMapper datasets.deepCoadd_calexp_hsc, disallowed key: level
      HscMapper datasets.deepDiff_metadata, missing key: template
      HscMapper datasets.deepDiff_kernelSrc, missing key: template
      HscMapper datasets.photoCalib, missing key: template

      obs_cfht

      MegacamMapper exposures.dcrCoadd_directWarp, missing key: template
      MegacamMapper exposures.deepDiff_matchedExp, missing key: template
      MegacamMapper exposures.deepDiff_differenceExp, missing key: template
      MegacamMapper datasets.donutSrc, missing key: template
      MegacamMapper datasets.deepDiff_kernelSrc, missing key: template
      MegacamMapper datasets.photoCalib, missing key: template
      MegacamMapper datasets.forcedPhotCcd_metadata, missing key: template
      MegacamMapper datasets.deepDiff_metadata, missing key: template
      MegacamMapper datasets.deepDiff_diaSrc, missing key: template

      obs_decam

      DecamMapper datasets.donutSrc, missing key: template
      DecamMapper datasets.photoCalib, missing key: template

      obs_lsstSim

      LsstSimMapper datasets.donutSrc, missing key: template
      LsstSimMapper datasets.photoCalib, missing key: template

      obs_sdss

      SdssMapper exposures.wcs, missing key: template
      SdssMapper exposures.raw, missing key: template
      SdssMapper exposures.dcrCoadd_directWarp, missing key: template
      SdssMapper datasets.forced_src, missing key: template
      SdssMapper datasets.forcedPhotCcd_metadata, missing key: template
      SdssMapper datasets.donutSrc, missing key: template
      SdssMapper datasets.photoCalib, missing key: template

      obs_test

      TestMapper exposures.deepCoadd_directWarp, missing key: template
      TestMapper exposures.deepCoadd_psfMatchedWarp, missing key: template
      TestMapper exposures.deepDiff_matchedExp, missing key: template
      TestMapper exposures.dcrCoadd_directWarp, missing key: template
      TestMapper datasets.photoCalib, missing key: template
      TestMapper datasets.forcedPhotCcd_metadata, missing key: template
      TestMapper datasets.deepDiff_diaSrc, missing key: template
      TestMapper datasets.deepCoadd_measMatchFull, missing key: template
      TestMapper datasets.srcMatchFull, missing key: template
      TestMapper datasets.deepDiff_metadata, missing key: template
      TestMapper datasets.deepCoadd_measMatch, missing key: template
      TestMapper datasets.donutSrc, missing key: template
      TestMapper datasets.deepDiff_kernelSrc, missing key: template

      MapperForTestCalexpMetadataObjects exposures.dcrCoadd_directWarp, missing key: template
      MapperForTestCalexpMetadataObjects exposures.deepDiff_differenceExp, missing key: template
      MapperForTestCalexpMetadataObjects exposures.deepDiff_matchedExp, missing key: template
      MapperForTestCalexpMetadataObjects exposures.icExp, missing key: template
      MapperForTestCalexpMetadataObjects exposures.deepCoadd_directWarp, missing key: template
      MapperForTestCalexpMetadataObjects exposures.raw, missing key: template
      MapperForTestCalexpMetadataObjects exposures.postISRCCD, missing key: template
      MapperForTestCalexpMetadataObjects exposures.deepCoadd_psfMatchedWarp, missing key: template
      MapperForTestCalexpMetadataObjects exposures.wcs, missing key: template

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            In some cases (donutSrc?) it may be appropriate to remove the dataset type from obs_base if it doesn't apply to all cameras.

            Show
            ktl Kian-Tat Lim added a comment - In some cases ( donutSrc ?) it may be appropriate to remove the dataset type from obs_base if it doesn't apply to all cameras.
            Hide
            rhl Robert Lupton added a comment - - edited

            If DM-8432 was setting policy like this, should it also have been an RFC? It isn't obvious to me that all cameras should be required to add an entry just because one without a default is added to obs_base.

            I think we need the equivalent of = delete in C++. Some cameras may well not want to define all of these data types. In fact, I'm surprised that some of these types are not defined in obs_base.

            I'd also like obs_base to be able to define some of these types as optional (e.g. few cameras will need donutSrc – e.g. SDSS never took doughnut images, and the camera is now in the Smithsonian. One way to do this would be to provide a default value, but if that's not desirable we still don't want all cameras to have to know about them.

            Show
            rhl Robert Lupton added a comment - - edited If DM-8432 was setting policy like this, should it also have been an RFC? It isn't obvious to me that all cameras should be required to add an entry just because one without a default is added to obs_base. I think we need the equivalent of = delete in C++. Some cameras may well not want to define all of these data types. In fact, I'm surprised that some of these types are not defined in obs_base . I'd also like obs_base to be able to define some of these types as optional (e.g. few cameras will need donutSrc – e.g. SDSS never took doughnut images, and the camera is now in the Smithsonian. One way to do this would be to provide a default value, but if that's not desirable we still don't want all cameras to have to know about them.
            Hide
            rhl Robert Lupton added a comment -

            If it applies to a significant fraction of cameras I think it should still be in obs_base, but ignorable by uninterested cameras.

            Show
            rhl Robert Lupton added a comment - If it applies to a significant fraction of cameras I think it should still be in obs_base, but ignorable by uninterested cameras.
            Hide
            npease Nate Pease added a comment -

            @rhl I don't know what you mean by "DM-8432 was setting policy like this". The policy changes were implemented as a result of RFC-204. At some point the decision to leave the template as "pure virtual"-ish in some cases was made in doing the work for RFC-204 (I'm not sure the history there, @pgee may be able to shed some light on that).

            I think what you're proposing is
            1. a way for an a dataset that is incomplete in obs_base to be marked as optional in obs_base, which has the effect that the policy for CameraMapper subclasses are not required to complete that policy, and the policy check will silently ignore the incomplete dataset type.
            2.The policy for CameraMapper subclasses may not want to declare policy for dataset types that are not marked as optional, and so they should be able to explicitly declare that datasetType as unused, which would then not require that dataset type to be completely defined.

            Is that right?

            ...is this over engineering the problem? I think the motivation for DM-8432 was an assumption that users would want an early failure if a policy was not fully populated. As it is currently, the failure will not occur until if and when the incomplete datasetType is used. The simplicity of not adding a bunch of rules for policy might make the late failure more desirable than rules + early failure. What do you think?

            Show
            npease Nate Pease added a comment - @rhl I don't know what you mean by " DM-8432 was setting policy like this". The policy changes were implemented as a result of RFC-204 . At some point the decision to leave the template as "pure virtual"-ish in some cases was made in doing the work for RFC-204 (I'm not sure the history there, @pgee may be able to shed some light on that). I think what you're proposing is 1. a way for an a dataset that is incomplete in obs_base to be marked as optional in obs_base, which has the effect that the policy for CameraMapper subclasses are not required to complete that policy, and the policy check will silently ignore the incomplete dataset type. 2.The policy for CameraMapper subclasses may not want to declare policy for dataset types that are not marked as optional, and so they should be able to explicitly declare that datasetType as unused, which would then not require that dataset type to be completely defined. Is that right? ...is this over engineering the problem? I think the motivation for DM-8432 was an assumption that users would want an early failure if a policy was not fully populated. As it is currently, the failure will not occur until if and when the incomplete datasetType is used. The simplicity of not adding a bunch of rules for policy might make the late failure more desirable than rules + early failure. What do you think?
            Hide
            rhl Robert Lupton added a comment -

            RFC-204 said "move datasets to obs_base"; that was and is a good idea. The point I made here was that it has some problems that we didn't think about, and proposed a way to fix it.

            Whether it's over engineering is something that we should discuss as part of this RFC.

            Show
            rhl Robert Lupton added a comment - RFC-204 said "move datasets to obs_base"; that was and is a good idea. The point I made here was that it has some problems that we didn't think about, and proposed a way to fix it. Whether it's over engineering is something that we should discuss as part of this RFC.
            Hide
            npease Nate Pease added a comment -

            An alternative approach I just discussed with @ktl would be to check the template when it is used and raise with a useful error message, something like "template must be defined for <datasetType> before it can be used".

            This way packages with mappers that derive from CameraMapper would not be required to implement all "pure virtual" templates, and the problem should be easy to identify when the dataset is used in get or put.

            Show
            npease Nate Pease added a comment - An alternative approach I just discussed with @ktl would be to check the template when it is used and raise with a useful error message, something like "template must be defined for <datasetType> before it can be used". This way packages with mappers that derive from CameraMapper would not be required to implement all "pure virtual" templates, and the problem should be easy to identify when the dataset is used in get or put .
            Hide
            rhl Robert Lupton added a comment -

            I think that'd be fine.

            Show
            rhl Robert Lupton added a comment - I think that'd be fine.
            Hide
            npease Nate Pease added a comment -

            We'll go with the "late failure with helpful error message" plan then.

            Show
            npease Nate Pease added a comment - We'll go with the "late failure with helpful error message" plan then.

              People

              • Assignee:
                npease Nate Pease
                Reporter:
                npease Nate Pease
                Watchers:
                Ian Sullivan, John Swinbank, Kian-Tat Lim, Nate Pease, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel