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

Custom classes for DataUnit tuples/sets and Data IDs

    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

            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Risk Score 0
            jbosch Jim Bosch made changes -
            Link This issue relates to DM-14821 [ DM-14821 ]
            jbosch Jim Bosch made changes -
            Assignee Jim Bosch [ jbosch ] Pim Schellart [ pschella ]
            nlust Nate Lust made changes -
            Link This issue relates to DM-15327 [ DM-15327 ]
            jbosch Jim Bosch made changes -
            Link This issue is blocked by DM-15336 [ DM-15336 ]
            pschella Pim Schellart [X] (Inactive) made changes -
            Assignee Pim Schellart [ pschella ] Nate Lust [ nlust ]
            jbosch Jim Bosch made changes -
            Link This issue relates to DM-15459 [ DM-15459 ]
            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.
            nlust Nate Lust made changes -
            Assignee Nate Lust [ nlust ] Jim Bosch [ jbosch ]
            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).
            jbosch Jim Bosch made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            jbosch Jim Bosch made changes -
            Story Points 4 10
            jbosch Jim Bosch made changes -
            Link This issue duplicates DM-15327 [ DM-15327 ]
            jbosch Jim Bosch made changes -
            Link This issue relates to DM-16310 [ DM-16310 ]
            jbosch Jim Bosch made changes -
            Link This issue duplicates DM-15679 [ DM-15679 ]
            jbosch Jim Bosch made changes -
            Link This issue blocks DM-15675 [ DM-15675 ]
            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.  
            jbosch Jim Bosch made changes -
            Reviewers Andy Salnikov, Christopher Waters [ salnikov, cwaters ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            jbosch Jim Bosch made changes -
            Epic Link DM-16675 [ 235235 ]
            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.
            salnikov Andy Salnikov made changes -
            Reviewers Andy Salnikov, Christopher Waters [ salnikov, cwaters ] Christopher Waters [ cwaters ]
            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).
            czw Christopher Waters made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            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.
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            jbosch Jim Bosch made changes -
            Link This issue relates to DM-17023 [ DM-17023 ]

              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:

                  Summary Panel