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

Add FrameDict class

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: astshim
    • Story Points:
      2
    • Epic Link:
    • Sprint:
      Alert Production F17 - 11
    • Team:
      Alert Production

      Description

      Add a new class FrameDict to astshim that is a FrameSet with a table for looking up frame index by domain. This can greatly simplify much of the code in afw that manipulates FrameSets.

        Attachments

          Issue Links

            Activity

            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-9679 [ 30784 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-10964 [ DM-10964 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-10765 [ DM-10765 ]
            rowen Russell Owen made changes -
            Reviewers Krzysztof Findeisen [ krzys ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            rowen Russell Owen made changes -
            Comment [ Thank you for agreeing to review this.

            I have two questions:
            - It turns out that AST forces all domain names to uppercase. I'm tempted to do the same with lookup, thus permitting lowercase names in the code, e.g.: {{frameSet.getIndex("foo")}} will return the index of the frame with domain "FOO". This is especially useful if one specifies the frame domain using a lowercase string, e.g. {code}
            {{frame = ast.Frame(2, "Domain=foo")
            frameDict = ast.FrameDict(frame)
            assert frameDict.getIndex("foo") == 1
            {code}. What do you think?
            - Do you think I should add two more overloads of {{FrameDict.getMapping}}: one for {{(fromIndex, toDomain)}} and the other {{(fromDomain, toIndex)}}? The argument in favor is that it means one can use a domain name with {{FrameDict}} *anywhere* an index is required by {{FrameSet}}. The arguments against are that it adds internal clutter, and code that mixes index and domain may look a bit odd, e.g. {{frameSet.getMapping(1, "FOO")}} (looks ugly to me) or {{frameSet.getMapping(frameSet.CURRENT, "FOO")}} (not so bad, in my opinion).
            ]
            Hide
            krzys Krzysztof Findeisen added a comment -

            I'm a bit concerned by the use of inheritance for the relationship between FrameDict and FrameSet, and by how easy it is to create an inconsistent FrameDict. I'd like to take another look once those have been addressed.

            Show
            krzys Krzysztof Findeisen added a comment - I'm a bit concerned by the use of inheritance for the relationship between FrameDict and FrameSet , and by how easy it is to create an inconsistent FrameDict . I'd like to take another look once those have been addressed.
            krzys Krzysztof Findeisen made changes -
            Status In Review [ 10004 ] In Progress [ 3 ]
            rowen Russell Owen made changes -
            Story Points 1 2
            Hide
            rowen Russell Owen added a comment - - edited

            Thank you for the very helpful review.

            I see your point about being able to use a FrameDict where a FrameSet is expected possibly having surprising results (inability to add a frame due to domain name conflict). However, I feel the risk of problems is low and does not justify the added code complexity. Conceptually a FrameDict is just a FrameSet that adds optional index-by-domain-name, and the class inheritance does reflect that.

            I strongly agree with your other points and I think I have made most your suggested changes. The results are a big improvement in safety, test coverage and (via copydoc) test coverage. See below.

            The changes I did not make are:

            • I did not remove statements such as using FrameSet::setCurrent because they are required to make the original method visible if the derived class defines a variant. I agree they are ugly.
            • I retained two math computations in unit tests, as I prefer to be more direct about the expected results instead of leaving it to FrameSet.

            In order to safely keep the dict in synch with the contained frames, for any operation that requires modifying the dict I make a copy of the contents as a FrameSet (which is what the underlying AST object is), modify that, then set the new dict and, if that succeeds, swap the AST objects. This is slower than the old code, but safer. Operations that modify the contained frames are expected to be rare, so I think we can easily live with the extra copying.

            I put all the changes on a new commit in the hope that it will make it easier to see what changes I made. I plan to squash it before (eventually) merging.

            Show
            rowen Russell Owen added a comment - - edited Thank you for the very helpful review. I see your point about being able to use a FrameDict where a FrameSet is expected possibly having surprising results (inability to add a frame due to domain name conflict). However, I feel the risk of problems is low and does not justify the added code complexity. Conceptually a FrameDict is just a FrameSet that adds optional index-by-domain-name, and the class inheritance does reflect that. I strongly agree with your other points and I think I have made most your suggested changes. The results are a big improvement in safety, test coverage and (via copydoc) test coverage. See below. The changes I did not make are: I did not remove statements such as using FrameSet::setCurrent because they are required to make the original method visible if the derived class defines a variant. I agree they are ugly. I retained two math computations in unit tests, as I prefer to be more direct about the expected results instead of leaving it to FrameSet . In order to safely keep the dict in synch with the contained frames, for any operation that requires modifying the dict I make a copy of the contents as a FrameSet (which is what the underlying AST object is), modify that, then set the new dict and, if that succeeds, swap the AST objects. This is slower than the old code, but safer. Operations that modify the contained frames are expected to be rare, so I think we can easily live with the extra copying. I put all the changes on a new commit in the hope that it will make it easier to see what changes I made. I plan to squash it before (eventually) merging.
            rowen Russell Owen made changes -
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            krzys Krzysztof Findeisen added a comment -

            Looks good. Unfortunately, some of my new comments are incorrectly flagged as "outdated", even though the code hasn't changed. Maybe it's a bug in how GitHub handles multi-commit filters.

            Show
            krzys Krzysztof Findeisen added a comment - Looks good. Unfortunately, some of my new comments are incorrectly flagged as "outdated", even though the code hasn't changed. Maybe it's a bug in how GitHub handles multi-commit filters.
            krzys Krzysztof Findeisen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Krzysztof Findeisen
                Watchers:
                Krzysztof Findeisen, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel