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

Custom classes for DataUnit tuples/sets and Data IDs

    XMLWordPrintable

    Details

      Description

      In the review for DM-14334 it looked like some utility classes for DataUnits could be helpful.  In particular:

      • We once had a DataUnitTuple class that defined a sort order and a de-duped set of value column names, but dropped it when we added the new DataUnit class.  Looks like we should create a new one that builds on the DataUnit class and TopologicalSet.
      • We should at least consider having a custom Data ID class, both to make it hashable and hence usable as a dict key and as a place to add integer packing in the future.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Note: DM-15214 adds some custom ordering code for DataUnits that should be obsoleted and replaced by this ticket.

            Show
            jbosch Jim Bosch added a comment - Note: DM-15214 adds some custom ordering code for DataUnits that should be obsoleted and replaced by this ticket.
            Hide
            jbosch Jim Bosch added a comment -

            Andy Salnikov, just a heads up that I'm starting on this now, and it'll involve a pretty thorough rewrite of the APIs for workings with DataUnits and DataUnitJoins, and hence it'll involve some big changes in the preflight code as well.

            I'll ping you again once I start working on the changes to preflight, as we probably want to avoid us both making changes there at the same time (but I'm not there yet).

            Show
            jbosch Jim Bosch added a comment - Andy Salnikov , just a heads up that I'm starting on this now, and it'll involve a pretty thorough rewrite of the APIs for workings with DataUnits and DataUnitJoins, and hence it'll involve some big changes in the preflight code as well. I'll ping you again once I start working on the changes to preflight, as we probably want to avoid us both making changes there at the same time (but I'm not there yet).
            Hide
            jbosch Jim Bosch added a comment - - edited

            This big ticket is ready for review, and  Andy Salnikov, I'm afraid I need to ask you to be the on to look at most of it (the changes in daf_butler), even though I already asked you to look at the first draft of some of this a week ago.

            Christopher Waters, could you take a look at the downstream changes to obs_base, skymap, pipe_base, pipe_tasks, and ci_hsc?  Only the first of those has more than trivial renames, and one specific question I have for you is whether the logic there is big-picture understandable without having to look at the docs for the new DataId object in daf_butler.

            Jonathan Sick, if you have time to look at the docstrings in daf.butler.dimensions I'd be grateful; there are a few things (**kwds arguments, plurals of symbols) in particular that I'm not sure I'm handling appropriately.

             

            Show
            jbosch Jim Bosch added a comment - - edited This big ticket is ready for review, and  Andy Salnikov , I'm afraid I need to ask you to be the on to look at most of it (the changes in daf_butler ), even though I already asked you to look at the first draft of some of this a week ago. Christopher Waters , could you take a look at the downstream changes to obs_base , skymap , pipe_base , pipe_tasks , and ci_hsc ?  Only the first of those has more than trivial renames, and one specific question I have for you is whether the logic there is big-picture understandable without having to look at the docs for the new DataId object in daf_butler. Jonathan Sick , if you have time to look at the docstrings in daf.butler.dimensions I'd be grateful; there are a few things (**kwds arguments, plurals of symbols) in particular that I'm not sure I'm handling appropriately.  
            Hide
            jbosch Jim Bosch added a comment -

            I'd previously forgotten about pipe_supertask, which also needed some (happily trivial) changes.  Now included as well.

            Show
            jbosch Jim Bosch added a comment - I'd previously forgotten about pipe_supertask , which also needed some (happily trivial) changes.  Now included as well.
            Hide
            salnikov Andy Salnikov added a comment -

            I checked daf_butler, looks OK in general and I think I managed to understand how it works  Small bunch of comments on PR. I have also approved pipe_base and pipe_supertask. Removing myself from reviewers, Christopher Waters, please mark it as reviewed once you are done with remaining packages.

            Show
            salnikov Andy Salnikov added a comment - I checked daf_butler , looks OK in general and I think I managed to understand how it works  Small bunch of comments on PR. I have also approved pipe_base and pipe_supertask . Removing myself from reviewers, Christopher Waters , please mark it as reviewed once you are done with remaining packages.
            Hide
            czw Christopher Waters added a comment -

            I've looked through the set excluding `daf_butler` (and also enough of `daf_butler` to convince myself I vaguely know how this works).  I think the `obs_base` code is generally understandable, although I've added comments on the pull request where I was either confused or unclear.  There are a handful of other small comments (typos and such).

            Show
            czw Christopher Waters added a comment - I've looked through the set excluding `daf_butler` (and also enough of `daf_butler` to convince myself I vaguely know how this works).  I think the `obs_base` code is generally understandable, although I've added comments on the pull request where I was either confused or unclear.  There are a handful of other small comments (typos and such).
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master.

            Thanks, all, for the fast and very helpful reviews.

            Show
            jbosch Jim Bosch added a comment - Merged to master. Thanks, all, for the fast and very helpful reviews.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Christopher Waters
              Watchers:
              Andy Salnikov, Christopher Waters, Jim Bosch
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.