Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-10542: Replace XYTransform::linearizeTransform #245

Merged
merged 3 commits into from Jun 16, 2017

Conversation

kfindeisen
Copy link
Member

The first two commits on this ticket implement testing-related RFCs, as recommended at the end of this Community post. They are not directly related to linearizeTransform.

The third commit adds a free function that takes a Transform and linearizes it. Per Effective C++, it's been placed in a different header from Transform itself; I expect the factory functions mentioned in RFC-346 will go in the same file once they're implemented.

@kfindeisen kfindeisen requested a review from r-owen June 15, 2017 20:58
*/
template <class T, int Rows, int Cols>
ndarray::Array<T, 2, 2> toNdArray(Eigen::Matrix<T, Rows, Cols> const &matrix) {
ndarray::Array<T, 2, 2> array = ndarray::allocate(ndarray::makeVector(matrix.rows(), matrix.cols()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be any more efficient to use the template parameters Rows and Colshere? I assume they *must* be identical tomatrix.rows()andmatrix.cols()`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, actually. Rows and Cols may have the dummy value -1 to mean "determined at run-time", and in fact they do have this value for the matrix returned by getJacobian. (Also, I'm pretty sure that in fixed-size matrices the implementations for rows() and cols() are optimized for inlining.)

tweakedInPoint = fromEndpoint.pointFromData(
rawLinPoint + deltaFrom[:, i])
assert_allclose(jacobian,
linearized.getJacobian(linPoint),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want the argument to be tweakedInPoint

err_msg=msg)
# First derivative will be tested in next section

# Is linearized linear?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding something to the effect: "Test by asserting that the Jacobian of the linearized transform measured at various points matches the Jacobian of the original transform at linPoint"

The function has been placed in a new transformFactory header that
will soon have other Transform-creating functions.
@kfindeisen kfindeisen merged commit c39b6bc into master Jun 16, 2017
@kfindeisen kfindeisen deleted the tickets/DM-10542 branch August 22, 2018 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants