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

Add boost to KerrSchild #5610

Merged
merged 1 commit into from
Jun 29, 2024
Merged

Conversation

guilara
Copy link
Contributor

@guilara guilara commented Nov 1, 2023

Proposed changes

Add a boost to the KerrSchild solution. This is done by boosting the coordinates, null form and derivatives of the H function defining the KerrSchild solution. (For the case when there is a boost the solution depends on time as well, but we don't code up the time dependence of the coordinates for simplicity.)

Evolving this solution will require time dependent translation maps to keep the excision surface inside the apparent horizon (#5576)

This reuses items from develop...nilsvu:spectre:kerr_boost

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@guilara guilara changed the title Add Boosted Kerr Initial Data Add Boosted KerrSchild Initial Data Nov 1, 2023
@guilara guilara force-pushed the PRs-KerrBoost branch 9 times, most recently from 624a213 to 1366cf9 Compare November 3, 2023 14:12
@guilara guilara marked this pull request as ready for review November 3, 2023 14:17
@guilara guilara changed the title Add Boosted KerrSchild Initial Data Add boost to KerrSchild Nov 3, 2023
@guilara guilara requested a review from nikwit November 10, 2023 10:24
Copy link
Contributor

@nikwit nikwit left a comment

Choose a reason for hiding this comment

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

nice work overall. Two points:

1.) I think we discussed that with a boost, KerrSchild is technically time dependent but you have only implemented it for t=0. It is therefore technically no longer a AnalyticSolution. Can you make sure to document this?

2.) As we discussed, could you write a test that computes tensors like the extrinsic curvature and the spacetime metric once boosted and once unboosted and make sure they correspond to a manual transformation?

* ## Boost of the Kerr-Schild solution
*
* We add initial momentum to the solution by applying a Lorentz boost to the
* metric. Given the form of the Kerr-Schild metric in terms of a covariant
Copy link
Contributor

@nikwit nikwit Nov 21, 2023

Choose a reason for hiding this comment

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

Can you rephrase this to something more readable? Perhaps "The Kerr-Schild metric can be expressed covariantly in terms of (...). We construct the boosted metric by boosting these object individually."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

internal_tags::x_minus_center<DataType, Frame> /*meta*/) const;

void operator()(const gsl::not_null<Scalar<DataType>*> a_dot_x,
const gsl::not_null<CachedBuffer*> cache,
void operator()(gsl::not_null<Scalar<DataType>*> a_dot_x,
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you delete all these const specifiers?

Copy link
Contributor Author

@guilara guilara Nov 21, 2023

Choose a reason for hiding this comment

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

clang-tidy was complaining. Said only has effect in the cpp

@@ -391,8 +450,7 @@ void KerrSchild::IntermediateComputer<DataType, Frame>::operator()(
const auto& lapse = get(cache->get_var(*this, gr::Tags::Lapse<DataType>{}));
const auto& lapse_squared =
get(cache->get_var(*this, internal_tags::lapse_squared<DataType>{}));
get(*deriv_lapse_multiplier) =
-square(null_vector_0_) * lapse * lapse_squared;
get(*deriv_lapse_multiplier) = -lapse * lapse_squared;
Copy link
Contributor

Choose a reason for hiding this comment

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

why can you set square(null_vector_0_) to 1 here but not other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think its set to one, just that it seems not in the formula for the spatial derivative of the lapse.

template <size_t SpatialDim>
tnsr::Ab<double, SpatialDim, Frame::NoFrame> lorentz_boost_matrix(
const std::array<double, SpatialDim>& velocity) {
tnsr::I<double, SpatialDim, Frame::NoFrame> velocity_as_tensor{};
Copy link
Contributor

Choose a reason for hiding this comment

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

tnsr::I<double, SpatialDim, Frame::NoFrame> velocity_as_tensor(velocity) should work I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Seems to work

/*!
* \ingroup SpecialRelativityGroup
* \brief Apply a Lorentz boost to the spatial part of a one form.
* \details The zero component of the one form is also specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "This requires passing the 0th component of the one form as an additional argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -58,6 +59,80 @@ void lorentz_boost_matrix(
(*boost_matrix).get(i + 1, i + 1) += 1.0;
}
}

template <size_t SpatialDim>
tnsr::Ab<double, SpatialDim, Frame::NoFrame> lorentz_boost_matrix(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make this a symmetric tensor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I? This has one upper and one lower index

Copy link
Member

Choose a reason for hiding this comment

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

Not currently supported.

}
const auto boost_matrix_1 = lorentz_boost_matrix(velocity_first_index);
const auto boost_matrix_2 = lorentz_boost_matrix(velocity_second_index);
for (size_t i = 0; i < SpatialDim + 1; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional] tenex::evaluate<ti::a, ti::b>(result, boost_matrix_1(ti::C, ti::a) * boost_matrix_2(ti::D, ti::b) * tensor(ti::c, ti::d) is quite concise.

* Note that the Lorentz matrix is symmetric.
*/
template <typename DataType, size_t SpatialDim, typename Frame>
void lorentz_boost(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, there would be one implementation that works for a generic tensor type so do not have all of these overloads. If you think that would be overkill for now, maybe just add a comment that this can be done in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I wouldn't want to delay this anymore. Added comment.

@@ -44,7 +44,7 @@ void compare_different_wrapped_solutions(const double mass,
template <typename SolutionType>
void test_copy_and_move(const SolutionType& solution) {
test_copy_semantics(solution);
auto solution_copy = solution;
const auto& solution_copy = solution;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is just a reference now. Can you just pass in solution directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -426,5 +426,55 @@ void verify_consistency(const Solution& solution, const double time,
derivative_delta),
derivative_approx);
}

/// Check the consistency of dependent quantities returned by a
Copy link
Contributor

Choose a reason for hiding this comment

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

dependent on what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On other metric quantities. Changed doc.

@guilara
Copy link
Contributor Author

guilara commented Feb 21, 2024

@nikwit I've added one more test to manually boost the spacetime metric and check that I get the same as the implementation. Not entirely sure how to do it for the extrinsic curvature. I think I need to upgrade it to 4d tensor (not sure about this) then boost it and compare a 4d tensor obtained from the implementation.

@guilara
Copy link
Contributor Author

guilara commented Feb 21, 2024

@nikwit was suggesting moving the KerrBoost to AnalyticData instead of AnalyticSolutions while I suggest leaving it here and add a warning in the doc.

The reasoning is mainly because the Kerr boost as it is currently implemented (and as it is implemented in SpEC) is only usable for initial data and some assumptions are being made. For instance, we are not coding up the time dependence at all (the solution now is both function of position and time since its moving, but we set t = 0), and assumptions on the time derivative of the lapse and shift (which will be rewritten anyway when evolved).

@nilsvu any thoughts on this?

@nilsvu
Copy link
Member

nilsvu commented Feb 21, 2024

@geoffrey4444 Could you review this one too, since you wrote the Lorentz transformations here and boosts in SpEC?

Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

Yes please keep this class in AnalyticSolution with the warning in the docs.

* metric. Since the Kerr-Schild metric can be expressed covariantly in terms of
* the Minkowski metric, a scalar function and a one form, we constructing the
* metric by boosting the these objects individually. Notice that we also need
* to appropriately boost the coordinates to the boosted frame.
Copy link
Member

Choose a reason for hiding this comment

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

I believe we're evaluating the boosted Kerr-Schild metric in the boosted frame, meaning that in these coordinates the black hole does not move, correct? So we inverse-boost the input coordinates to get to the unboosted frame, evaluate Kerr-Schild quantities in the unboosted frame, then boost these quantities. Please clarify the docs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will clarify. I got confused about it myself at some point. We start with coordinates in the frame where the black hole is moving. Then boost them to the frame where the black hole is at rest, and where we can construct the stationary solution. Then we boost all covariant quantities back to the frame where it is moving.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, for initial data I thought we're solving in a frame where the BHs are at rest.. @geoffrey4444 can you clarify how this works in SpEC?

Copy link
Member

Choose a reason for hiding this comment

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

Talked about this in the meeting today. I think you're correct. For initial data we go into a rotating frame that gets rid of the translation but keeps the Lorentz contraction.

* \warning While technically the boosted Kerr-Schild metric is dependent on
* both the time and space coordinates, we have implemented it only at $t = 0$
* as in SpEC. Therefore it is technically not an analytic solution and should
* not be used to compute errors with respect to it.
Copy link
Member

Choose a reason for hiding this comment

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

The solution should be stationary in the boosted frame; so shouldn't all time derivatives be zero and this be a solution at any $t$? Am I misunderstanding something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is stationary in the rest frame, which is not the frame we want. In the frame we want, the black hole is moving and so the metric will be dependent on t and x. I didn't code this t dependence since I didn't see this in SpEC --it seems to me it should only enter in the function that computes the radius to the center.

I also didn't code the time derivatives of the lapse and shift in the frame where the black hole is moving (which are zero in the rest frame of course) but left them as zero. This should be fine because I think they are replaced by the gauge functions we use to evolve this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I think you're correct. Let me look at the SpEC implementation too.

@@ -58,6 +59,80 @@ void lorentz_boost_matrix(
(*boost_matrix).get(i + 1, i + 1) += 1.0;
}
}

template <size_t SpatialDim>
tnsr::Ab<double, SpatialDim, Frame::NoFrame> lorentz_boost_matrix(
Copy link
Member

Choose a reason for hiding this comment

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

Not currently supported.

/// @{
/*!
* \ingroup SpecialRelativityGroup
* \brief Apply a Lorentz boost to the spatial part of a one form.
Copy link
Member

Choose a reason for hiding this comment

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

The argument type is a vector (tnsr::I), so I also think this documentation should read vector. Refer to the docs of lorentz_boost_matrix above for details and math. I also find those docs confusing though. I believe they say that $\Lambda^a_\bar{a}$ is computed, which boosts a vector from barred to unbarred or a one-form from unbarred to barred (consistent with Blandford/Thorne Sec. 2.7). So my suggestion is:

  • The docs and argument for this function should read vector (they boost a vector)
  • The function for the one-form below should apply the inverse Lorentz transformation (by inverting the sign of the velocity). This means you add a minus sign to the function below and remove the minus signs from where the function is used in Kerr-Schild. Make sure to clarify this in the docs of the function.

@geoffrey4444 can you confirm or clarify this?

@@ -366,13 +380,116 @@ void test_zero_spin_optimization(const DataType& used_for_size) {
});
}

template <typename Frame, typename DataType>
void test_boosted_for_zero_velocity(const DataType& used_for_size) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice test!

Comment on lines +425 to +445
// Need to evaluate at the boosted coordinates
// Boost coordinates
Copy link
Member

Choose a reason for hiding this comment

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

These are the unboosted coords, no? (related to comments above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the terminology ''boosted/unboosted'' is a bit confusing. Rest frame (of the BH) coords I would say.


template <typename Frame, typename DataType>
void test_boosted_spacetime_metric(const DataType& used_for_size) {
// Test that the extrinsic curvature of the boosted solution indeed
Copy link
Member

Choose a reason for hiding this comment

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

extrinsic curvature -> spacetime metric? Also test extrinsic curvature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet sure how to do it. I think I would need to elevate the extrinsic curvature to a 4d tensor in both frames.

@@ -86,3 +167,9 @@ SPECTRE_TEST_CASE(
d = large_velocity_squared;
CHECK_FOR_DOUBLES(test_lorentz_boost_matrix_analytic, (1, 2, 3));
}

SPECTRE_TEST_CASE("Unit.PointwiseFunctions.SpecialRelativity.LorentzBoost",
Copy link
Member

Choose a reason for hiding this comment

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

combine with test case above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined

@geoffrey4444
Copy link
Contributor

@geoffrey4444 Could you review this one too, since you wrote the Lorentz transformations here and boosts in SpEC?

I'll try but really swamped this week.

@guilara
Copy link
Contributor Author

guilara commented Jun 26, 2024

@nilsvu I have rebased this and added an assert that would throw an error if we call a tag I can't compute for the boost.

Copy link
Contributor

@geoffrey4444 geoffrey4444 left a comment

Choose a reason for hiding this comment

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

@nilsvu does this look ready to go to you?

Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for fixup this up. Pleas squash your commits. Also to keep the git blame record straight, could you add this to your commit message:

# Two empty lines


Co-authored-by: Nils Vu <nilsvu@caltech.edu>

@nilsvu
Copy link
Member

nilsvu commented Jun 29, 2024

It's two empty lines, no > symbols

Add more Lorentz boost functions

Add instantiation of time deriv of spatial metric

Add boosted Kerr initial data

Expand doc

Fix initialization of lorentz boost matrix

Remove unused reference

Add test of boosted spacetime metric

Throw error for unsupported tag

Co-authored-by: Nils Vu <nilsvu@caltech.edu>
@guilara
Copy link
Contributor Author

guilara commented Jun 29, 2024

It's two empty lines, no > symbols

Oh, sure

@nilsvu nilsvu merged commit a18184a into sxs-collaboration:develop Jun 29, 2024
22 checks passed
@nilsvu
Copy link
Member

nilsvu commented Jun 29, 2024

Thank you for getting this done @guilara !

@guilara
Copy link
Contributor Author

guilara commented Jul 1, 2024

Thank you for getting this done @guilara !

Thank you for all the detailed help with it

ctrandrsn pushed a commit to ctrandrsn/spectre-ctr that referenced this pull request Jul 8, 2024
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.

4 participants