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

            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.
            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.
            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.

              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