DM-9925: PolyTran should not provide an iterative inverse by default #6
Conversation
PolyMap tried to provide an iterative inverse by default if no inverse coefficients were provided. This proved confusing, and resulted in incorrect answers if the polynomial was not invertible. Change the default, update the documentation and provide more tests.
Also slightly beef up the test of Frame.convert. Neither is a very rigorous test since both use a trivial case.
It's not entirely clear it is necessary but it can be handy. Also it is referred to in AST examples.
include/astshim/PolyMap.h
Outdated
|
|
||
| If coeff_i is empty then no inverse transformation is provided, | ||
| unless you specify suitable options to request an iterative inverse | ||
| (see the other constructor for details). |
There was a problem hiding this comment.
A link might be useful here, possibly
@ref PolyMap(ndarray::Array<double, 2, 2> const &, int, std::string const &) "the other constructor"
to mask the complex signature.
include/astshim/PolyMap.h
Outdated
| @@ -52,6 +52,9 @@ friend class Object; | |||
| /** | |||
| Construct a @ref PolyMap with specified forward and inverse transforms. | |||
|
|
|||
| The two sets of coefficients are independent of each other: the inverse transform | |||
| need not undo the forward transform. | |||
|
|
|||
| @param[in] coeff_f A `(2 + nin) x ncoeff_f` matrix of coefficients. | |||
There was a problem hiding this comment.
Something that came up when working on DM-9598: should this be (2 + nin) x ncoeff_f, or ncoeff_f x (2 + nin)? When making an ndarray::Array to pass here it appears you need to give the dimensions in the latter order.
| [-43.5, 1309.31], | ||
| ]) | ||
| outdata = fset.tranForward(indata) | ||
| assert_allclose(outdata, indata) |
There was a problem hiding this comment.
Can frame.convert be tested with something other than the identity transformation?
There was a problem hiding this comment.
The test for Frame.convert is quite weak, but I suggest another ticket for that. In general tests of the more complicated methods and approximation methods tend to be trivial, intended more as a sanity check that the wrapping was done correctly than as a torture test of the algorithm. It would be good to beef up the tests.
include/astshim/PolyMap.h
Outdated
| @@ -71,23 +74,38 @@ friend class Object; | |||
|
|
|||
| Each final output coordinate value is the sum of the terms described | |||
| by the `ncoeff_f` columns in the supplied array. | |||
|
|
|||
| If coeff_f is empty then no forward transformation is provided. | |||
There was a problem hiding this comment.
These paragraph breaks are a bit problematic: https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/classast_1_1_poly_map.html#aa45fef22f3c82b8bb8e2549c5de097cd.
I'd recommend moving the discussion of how coeff_* needs to be formatted to the main description, and restricting the descriptions of coeff_f and coeff_i to something that will fit in a single paragraph.
| ]) | ||
| des_pout = 2.0*pin**2 - pin**3 | ||
| pout = pm.tranForward(pin) | ||
| npt.assert_allclose(pout, des_pout) |
There was a problem hiding this comment.
The names here are a bit confusing. I'm guessing p stands for point, but it took me about a minute to think of that; spelling it out or using x/y (as in the docstring) would be clearer.
Likewise, what's des_ stand for?
There was a problem hiding this comment.
I agree that pin is not a great name; indata or inarr would probably be better. However, fixing that is out of scope for this ticket. In not keen to use x / y since it breaks down for large numbers of axes (which doc string refers to those names?).
des is short for desired.
A good time to fix the names is when I stop transposing data in astshim, since I will have to modify the input and expected output data for most of the unit tests.
There was a problem hiding this comment.
The docstring for test_PolyMapPolyMapUnivertible, line 229 (hope the link doesn't break...)
MapBox will now throw an exception if maxOutCoord < 0 (as it already did if maxInCoord < 0).
| << "], or 0 for all remaining"; | ||
| throw std::invalid_argument(os.str()); | ||
| } | ||
| } else if ((maxOutCoord < 0) || (maxOutCoord > nout)) { |
There was a problem hiding this comment.
You're doing input validation on a public modifiable field...?
There was a problem hiding this comment.
No, on the input argument. It's a struct-like class though the fields probably should be const.
That said, i think the macOutCoord field is not properly set if the input is 0. I'll fix that on a separate ticket. Also, this function probably should only take the map as an argument.
Fixed all Doxygen warnings I could fix except documenting =default methods. This leaves one invalid reference to the CardType enum in attributes_fitsChan.dox and a host of warnings about deleted methods due to using an outdated version of Doxygen.
No description provided.