-
Notifications
You must be signed in to change notification settings - Fork 192
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
Compute ADM momenta as volume integrals #6433
base: develop
Are you sure you want to change the base?
Conversation
11ba5c0
to
c656ebb
Compare
// Skip interfaces not at the outer boundary | ||
if (boundary_direction != Direction<3>::upper_zeta()) { | ||
continue; | ||
// Interfaces at the inner boundary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this to avoid duplicating all these projections:
if (boundary_direction.dimension() != 2) {
continue;
}
// Do projections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out! I've also removed the duplicated code for the face normals and area elements.
@@ -54,7 +54,7 @@ void test_local_adm_integrals(const double& distance, | |||
const std::vector<double>& prev_distances) { | |||
// Define black hole parameters. | |||
const double mass = 1; | |||
const double boost_speed = 0.5; | |||
const double boost_speed = 0.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the volume integrals give incorrect results for a boosted Schwarzschild case. In the past, we've wondered if this is due to some falloff assumption from Gauss Law being broken.
@@ -198,6 +198,8 @@ void test_local_adm_integrals(const double& distance, | |||
deriv_conformal_metric, inv_conformal_metric); | |||
const auto conformal_christoffel_contracted = tenex::evaluate<ti::i>( | |||
conformal_christoffel_second_kind(ti::J, ti::i, ti::j)); | |||
const auto deriv_conformal_christoffel_second_kind = partial_derivative( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this and the longitudinal shift, Ricci additions below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these quantities were only needed for the ADM mass volume integral, which isn't being computed here. Thanks for catching that!
Proposed changes
This PR improves the accuracy of P_ADM and J_ADM by computing them as a sum of infinite volume integrals and finite surface integrals, which were derived from the standard ADM equations using Gauss' Law. We have attempted to use this same trick for other asymptotic quantities (ADM mass/energy and center of mass), but the volume version didn't improve accuracy. This is evident from the convergence plot below, in which the CoM volume results have been omitted because of blow-ups.
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments