# Allow Transform to return its inverse

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
4
• Sprint:
Alert Production S17 - 3
• Team:

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

## Activity

Hide
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 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
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
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
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
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
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
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
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
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
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
Krzysztof Findeisen added a comment -

Merged with requested enhancements.

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

## People

• Assignee:
Krzysztof Findeisen
Reporter:
Krzysztof Findeisen
Reviewers:
Russell Owen
Watchers:
Krzysztof Findeisen, Russell Owen