Numerically invert Jacobian in combined call #6223
Conversation
| determinant_and_inverse(jac).second, std::move(jac), | ||
| std::move(frame_velocity)}; |
There was a problem hiding this comment.
Add in the CoordinateMap.hpp docs for this function that we don't call the individual inverse_jacobian functions and multiply them together, but rather just compute the jacobian and numerically invert it because most of our production time-dependent maps do this already.
There was a problem hiding this comment.
Is there a reason to keep the inverse_jacobian implementations on the individual maps at all? Doesn't need to be done in this PR, but removing them would be nicer than documenting that they are unused.
There was a problem hiding this comment.
Well they are still used in the individual inverse_jacobian function of CoordinateMap{Base}, correct? Whether or not that function is actually used in practice, I don't know. If not, then yes we can probably get rid of it
There was a problem hiding this comment.
Or we could make it do the numerical inversion like this function now does.
There was a problem hiding this comment.
Yeah we could! Let's discuss on the Monday call
There was a problem hiding this comment.
Another thing for a separate PR: this function could also return the Jacobian determinant. We need this a bit in the hydro cases.
There was a problem hiding this comment.
From the Monday call: We can just numerically invert the jacobian in the individual function as well which means we don't need the individual inverse_jacobian functions. See #6233
| determinant_and_inverse(jac).second, std::move(jac), | ||
| std::move(frame_velocity)}; |
There was a problem hiding this comment.
Another thing for a separate PR: this function could also return the Jacobian determinant. We need this a bit in the hydro cases.
nikwit
force-pushed
the
speed-up-coord-map
branch
from
bc43652 to
e442112
Compare
|
I added a comment about the inverse Jacobian being inverted numerically in |
|
Sounds good. Probably would require updating some tags. I think it's low priority :) Thank you! |
In some maps like
ShapeandRotScaleTrans, the inverse jacobian is compute by computing the jacobian and inverting it numerically. This means that for BBH simulations, we currently compute these jacobians twice, invert them and multiply them together. In this PR, this is changed so that only the jacobian is computed for all maps and the final jacobian is inverted.For BBH simulations, I found this speeds up the simulation between 10% and 18%, depending on the number of l-modes.