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

Allow Transform to return its inverse

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      Description

      The Transform class being introduced in DM-8439 is composed of (potentially) invertible Mappings, but is not itself invertible. A getInverse method should be added to expose this functionality.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Proposed API:

            /**
             * The inverse of this Transform.
             *
             * @returns a Transform whose `tranForward` is equivalent to this Transform's
             *          `tranInverse`, and vice versa.
             *
             * @exceptsafe Provides strong exception safety.
             */
            Transform<ToEndpoint, FromEndpoint> getInverse() const;
            

            (Note: I don't understand AST well enough to know whether or why an invert attempt can throw runtime_error). I acknowledge that AST errors cannot be sensibly encapsulated. Stupid global flag...

            EDIT: removed spec for memory errors, because I forgot that AST/astshim does not necessarily throw bad_alloc for dynamic memory allocations. Disclaimer re: non-invertible Transforms will be moved to class-level documentation, as part of an explanation of how forward and inverse transforms work.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Proposed API: /** * The inverse of this Transform. * * @returns a Transform whose `tranForward` is equivalent to this Transform's * `tranInverse`, and vice versa. * * @exceptsafe Provides strong exception safety. */ Transform<ToEndpoint, FromEndpoint> getInverse() const; (Note: I don't understand AST well enough to know whether or why an invert attempt can throw runtime_error ). I acknowledge that AST errors cannot be sensibly encapsulated. Stupid global flag... EDIT: removed spec for memory errors, because I forgot that AST/astshim does not necessarily throw bad_alloc for dynamic memory allocations. Disclaimer re: non-invertible Transforms will be moved to class-level documentation, as part of an explanation of how forward and inverse transforms work.
            Hide
            rowen Russell Owen added a comment -

            Please skip @throws MemoryError. Any object creation can do that, but it is very unlikely because such objects are small. Even trivial getX methods can run out of memory if they return a copy and I don't think it is reasonable to document that everywhere.

            Mapping.getInverse makes a deep copy and sets the invert flag on it, and neither of these operations has a known reason to fail, so no @throw notation is called for. That said, it is certainly possible to make Mapping.getInverse throw a runtime_error. It just takes work (e.g. calling AST directly and putting it into an error state) or an astshim bug (forgetting to check for the error state somewhere).

            Also I think the note should be reworded for clarity. Any mapping may have a valid forward transform, a valid inverse transform or both. Calling Mapping.getInverse returns a deep copy of the mapping with its internal Invert flag toggled. This maps which internal transform is called by tranForward and tranInverse.

            Note that astshim Mapping methods include getTranForward (does the forward transform exists?), getTranInverse (does the inverse transform exist?) and isInverted (is the internal Invert flag set?). You may want to provide access to these, though preferably using a different name for the first two.

            Show
            rowen Russell Owen added a comment - Please skip @throws MemoryError . Any object creation can do that, but it is very unlikely because such objects are small. Even trivial getX methods can run out of memory if they return a copy and I don't think it is reasonable to document that everywhere. Mapping.getInverse makes a deep copy and sets the invert flag on it, and neither of these operations has a known reason to fail, so no @throw notation is called for. That said, it is certainly possible to make Mapping.getInverse throw a runtime_error. It just takes work (e.g. calling AST directly and putting it into an error state) or an astshim bug (forgetting to check for the error state somewhere). Also I think the note should be reworded for clarity. Any mapping may have a valid forward transform, a valid inverse transform or both. Calling Mapping.getInverse returns a deep copy of the mapping with its internal Invert flag toggled. This maps which internal transform is called by tranForward and tranInverse . Note that astshim Mapping methods include getTranForward (does the forward transform exists?), getTranInverse (does the inverse transform exist?) and isInverted (is the internal Invert flag set?). You may want to provide access to these, though preferably using a different name for the first two.
            Hide
            rowen Russell Owen added a comment -

            Overall this looks excellent to me. I had two substantive suggestions:

            • Please construct the inverse transform using a frame set instead of a mapping, in order to preserve information.
            • Consider implementing DM-9826 as part of this ticket, because it is trivial and provides useful capability for testing getInverse.
            Show
            rowen Russell Owen added a comment - Overall this looks excellent to me. I had two substantive suggestions: Please construct the inverse transform using a frame set instead of a mapping, in order to preserve information. Consider implementing DM-9826 as part of this ticket, because it is trivial and provides useful capability for testing getInverse.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Russell Owen, can you take another look at the PR? The DM-9826 commit required some reorganization of the testing code to get it to work with non-invertible transforms, so I may have mixed something up.

            Show
            krzys Krzysztof Findeisen added a comment - Hi Russell Owen , can you take another look at the PR? The DM-9826 commit required some reorganization of the testing code to get it to work with non-invertible transforms, so I may have mixed something up.
            Hide
            rowen Russell Owen added a comment - - edited

            I strongly encourage to you also rename hasTranForward/Inverse in astshim as well (on the same ticket). I see no reason the names ought to be different, and keeping them the same in both packages will pay dividends in lack of confusion.

            Please also consider making the endpoint equality test available in C++ (if it's not a lot more work). It seems safer than trying to justify why it is only available in Python, and the code is very similar, as implemented (the code could be dramatically simplified if it was pure Python).

            Other than that it looks great. I had a few minor suggestions on gihub.

            Show
            rowen Russell Owen added a comment - - edited I strongly encourage to you also rename hasTranForward/Inverse in astshim as well (on the same ticket). I see no reason the names ought to be different, and keeping them the same in both packages will pay dividends in lack of confusion. Please also consider making the endpoint equality test available in C++ (if it's not a lot more work). It seems safer than trying to justify why it is only available in Python, and the code is very similar, as implemented (the code could be dramatically simplified if it was pure Python). Other than that it looks great. I had a few minor suggestions on gihub.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Merged with requested enhancements.

            Show
            krzys Krzysztof Findeisen added a comment - Merged with requested enhancements.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel