DM-9792: Mapping::getInverse not exception-safe #3

r-owen · 2017-03-15T18:23:56Z

No description provided.

If the call to `assertOK` in Mapping.getInverse threw an exception then the AST object `rawCopy` would never be deallocated.

kfindeisen

I have some comments/corrections to the documentation, otherwise looks good.

kfindeisen · 2017-03-15T18:34:24Z

doc/mainpage.dox

@@ -45,7 +45,7 @@ that support time, spectra and tables have yet been wrapped. The python wrapping
    - After calling AST code the wrapper checks AST's error state and if invalid,
        the wrapper resets the AST status code and throws `std::runtime_error`.
        At present the exception contain an error number, rather than useful text,
-        and useful text is printed to stderr. I hope to fix that at some point.
+        and useful text is printed to stderr. Work in in progress to fix this.


Work is in progress

kfindeisen · 2017-03-15T21:04:58Z

doc/mainpage.dox

 - Methods that output floating point data have `AST__BAD` replaced with `nan`.

 ### Smaller differences (not a complete list):

- Methods such as @ref Object.set do not support printf formatting and extra arguments.
+- Methods such as @ref Object.set and the attributes argument of constructors do not support


In general we don't need @ref anymore, only for all-lowercase unqualified names.

Good to know. But cleaning up all the usage in this package would be quite a chore. I'm inclined to leave it as explicit.

kfindeisen · 2017-03-15T21:07:22Z

include/astshim/Mapping.h

-    Get @ref Mapping_TranForward "TranForward": is the forward transform defined?
+    Is the forward transform available?
+
+    Note: this gets the @ref Mapping_TranForward "TranForward" attribute,


Prefer the @note tag to plain text. Same for hasTranInverse.

Fair enough. I'll change it though I personally dislike the resulting HTML formatting produced by Doxygen.

I guess we could ask @jonathansick to make it less prominent in the breathe output. I think the styling is still TBD.

kfindeisen · 2017-03-15T21:19:04Z

include/astshim/Mapping.h

+    Note that the inverse mapping contains exactly the same model coefficients as the original,
+    but they are used by @tranInverse instead of @tranForward. Thus for example if a @ref ZoomMap
+    has a zoom factor of 4.0 then its inverse also reports a zoom factor of 4.0 (despite behaving
+    like an uninverted @ref ZoomMap with zoom factor of 0.24).


Good grief. This is definitely unintuitive, 👍 for documenting it.

Should the final coefficient be 0.25?

Our existing XYTransform works almost the same way, but inversion is handled by a class that contains another transform, which is arguably a simpler model. In both cases the model coefficients are left unchanged. That is easier than trying to work out new model coefficients, especially for complex mappings such as PolyMap that may not even have an analytic inverse.

I think @kfindeisen is saying that 0.24 should be 0.25.

And yes, there's nothing to gain from the shim layer trying to recalculate the coefficients itself when AST is already doing that internally.

kfindeisen · 2017-03-15T21:24:14Z

include/astshim/PolyMap.h

@@ -134,7 +136,7 @@ friend class Object;
        return std::static_pointer_cast<PolyMap>(_copyPolymorphic());
    }

-    /// Get @ref PolyMap_IterInverse "IterInverse": provide an iterative inverse transformation?
+    /// Get @ref PolyMap_IterInverse "IterInverse": does this provide an iterative inverse transformation?


I think I prefer the style you adopted for Mapping::hasTranForward, i.e., make the brief description plain English and mention the equivalent property in the detailed description.

I assume these getters can't be noexcept because getB might throw over a completely unrelated error?

I have used the style above for all getters and setters throughout astshim with a few well-motivated minor exceptions. Changing it would be a bigger job than I want to tackle right now.

Correct that they cannot be noexcept.

kfindeisen · 2017-03-15T21:28:52Z

include/astshim/PolyMap.h

@@ -289,7 +291,7 @@ friend class Object;
                          ncoeff_i, coeff_i.getData(), options.c_str());
    }

-    /// Make a raw AstPolyMap with only a forward transform.
+    /// Make a raw AstPolyMap with a specified forward transform and a numeric approximation inverse.


The grammar seems a bit weird. Maybe "a numerically approximated inverse"?

kfindeisen · 2017-03-15T21:41:53Z

tests/test_polyMap.py

+            xn = 1.2 * xi * xi - 0.5 * yi * xi
+            yn = yi
+            self.assertAlmostEqual(xn, xo)
+            self.assertAlmostEqual(yn, yo)


This code is kind of hard to read. Maybe if you renamed pin and pout (I think they're data, not coefficients, right)? Maybe also xn -> xExpect?

That was bothering me, too. I cleaned it up quite a bit and simplified it by using numpy.test.assert_allclose.

kfindeisen · 2017-03-15T21:43:10Z

tests/test_polyMap.py

+            [-0.5, 1, 1, 1],
+            [1.0, 2, 0, 1],
+        ])
+        pm = astshim.PolyMap(coeff_f, 2, "IterInverse=0")


I assume the stringly typed code is an AST-ism. Are there any plans to replace this with some kind of preferences object (e.g., a map)?

You mean the IterInverse=0 string? That's part of the AST API but it can also accept printf-style formatting. AST won't care if astshim builds the options internally (the Perl and pyast interfaces both support alternate ways of specifying the key/value options). Sometimes the string is clearer though.

I do not like specifying values as an option string, but as @timj says, it is part of the AST API and not a part I felt could reasonably be hidden by astshim.

does astshim support the printf-options?

No. C++ string formatting is simple enough without it and Python string formatting is nearly trivial. Is there a use case I am missing for printf style formatting? It's not type safe and I would much rather avoid it if possible.

kfindeisen · 2017-03-15T21:55:46Z

tests/test_polyMap.py

+        self.assertTrue(pminv.isInverted())
+
+        pout2 = pminv.tranInverse(pin)
+        # pout and pout2 should be numerically identical because inverting


"numerically identical" -> "exactly equal"?

- Explain Mapping.getInverse in detail. - Provide links to useful attributes in PolyMap's constructor that only specifies the forward transform. - Tweak mainpage.dox.

- Test a PolyMap with no inverse transform (this test turned up a bug in AST which is fixed in AST 8.5.0). - Test the default iterative inverse. - Test setting attributes at construction.

and similarly for getTranInverse

Fix a memory leak in Mapping.getInverse

3cf1e80

If the call to `assertOK` in Mapping.getInverse threw an exception then the AST object `rawCopy` would never be deallocated.

kfindeisen approved these changes Mar 15, 2017

View reviewed changes

r-owen added 3 commits March 15, 2017 16:40

Improve documentation

3507ba2

- Explain Mapping.getInverse in detail. - Provide links to useful attributes in PolyMap's constructor that only specifies the forward transform. - Tweak mainpage.dox.

Improve test_polyMap.py

59a70fd

- Test a PolyMap with no inverse transform (this test turned up a bug in AST which is fixed in AST 8.5.0). - Test the default iterative inverse. - Test setting attributes at construction.

Rename Mapping.getTranForward to hasTranForward

a38aba5

and similarly for getTranInverse

r-owen force-pushed the tickets/DM-9792 branch from 576fb01 to a38aba5 Compare March 15, 2017 23:41

r-owen merged commit a38aba5 into master Mar 16, 2017

ktlim deleted the tickets/DM-9792 branch August 25, 2018 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9792: Mapping::getInverse not exception-safe #3

DM-9792: Mapping::getInverse not exception-safe #3

r-owen commented Mar 15, 2017

kfindeisen left a comment

kfindeisen Mar 15, 2017

kfindeisen Mar 15, 2017 •

edited

r-owen Mar 15, 2017

kfindeisen Mar 15, 2017

r-owen Mar 15, 2017 •

edited

kfindeisen Mar 15, 2017

kfindeisen Mar 15, 2017

kfindeisen Mar 15, 2017

r-owen Mar 15, 2017 •

edited

timj Mar 15, 2017

kfindeisen Mar 15, 2017

kfindeisen Mar 15, 2017

r-owen Mar 15, 2017

kfindeisen Mar 15, 2017

kfindeisen Mar 15, 2017

r-owen Mar 15, 2017

kfindeisen Mar 15, 2017

timj Mar 15, 2017 •

edited

r-owen Mar 15, 2017

timj Mar 15, 2017

r-owen Mar 15, 2017 •

edited

kfindeisen Mar 15, 2017

DM-9792: Mapping::getInverse not exception-safe #3

DM-9792: Mapping::getInverse not exception-safe #3

Conversation

r-owen commented Mar 15, 2017

kfindeisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Mar 15, 2017 •

edited

r-owen Mar 15, 2017 •

edited

r-owen Mar 15, 2017 •

edited

timj Mar 15, 2017 •

edited

r-owen Mar 15, 2017 •

edited