-
Notifications
You must be signed in to change notification settings - Fork 189
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 volume integrands for XCTS asymptotic quantities. #6276
Add volume integrands for XCTS asymptotic quantities. #6276
Conversation
6175d15
to
07af552
Compare
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.
I haven't reviewed the tests yet. Please push changes as fixup commits.
* \bar\gamma^{jk} \partial_i \bar\Gamma^i_{jk} | ||
* + \bar\Gamma^i_{jk} \partial_i \bar\gamma^{jk} | ||
* - \bar\gamma^{ij} \partial_i \bar\Gamma_j | ||
* - \bar\Gamma_j \partial_i \bar\gamma^{ij} |
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.
I think this is missing 2 terms:
I don't think these cancel since
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.
If I understand it right, these extra terms would only show up if we take the covariant derivative
That said, after redoing the calculations, I found an extra term involving the conformal factor first derivative:
Again, this is only if we use
I'm not sure what approach we should take here, but both of them require updates to these integrands.
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 need the covariant Laplacian of
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.
Ok, that makes sense. This raises another point. If we're using
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.
For Gauss' law you transform
So, you integrate with the curved area/volume element, get a covariant derivative from Gauss' law, and expand it into Christoffel terms where 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.
This is helpful, thanks! The test that I had in Test_AdmMass
was using flat area/volume elements and started failing after these changes. Once I updated this test to use curved area/volume elements, I started getting the correct values again. So, things seem consistent.
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.
Ok great! Both approaches work, we just have to be consistent. Please note in the docs for the integrand function if it needs to be integrated with the flat or conformal area/volume element.
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 squash these two commits (including this change)
14d7a3a
to
215dab7
Compare
Looks good, you can squash the fixup. |
0b09e1f
to
016f6ed
Compare
New fixups look good too, you can squash |
016f6ed
to
738d505
Compare
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.
Looks good. You can squash everything, including this.
@@ -195,6 +196,15 @@ 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.
At the moment you don't use this yet, right? Same for Ricci scalar/tensor and longitudinal shift below. Maybe hold off with adding these until you use them in this test.
} | ||
} | ||
} | ||
|
||
// Check result | ||
auto custom_approx = Approx::custom().epsilon(10. / distance).scale(1.0); | ||
auto custom_approx = Approx::custom().epsilon(TOLERANCE).scale(1.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.
Can you keep this at O(1/distance)?
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.
It doesn't work for this test because the CoM integral "diverges" as we increase R. I believe this happens due to round-off errors when canceling larger and larger terms. I describe a bit of this at the top of this file.
|
||
// Set up domain | ||
const size_t h_refinement = 1; | ||
const size_t p_refinement = 11; |
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.
This is very high refinement. My understanding is that the accuracy of the integral is currently not limited by resolution, so can you lower this?
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.
This was the lowest resolution that I was able to find in order to keep the tolerance at
I think there must be some change that we can make in the calculation of the CoM integrals to get around these round-off errors, but this is probably something for a future PR.
614cb85
to
42206b6
Compare
ADM mass test times out, probably just relax the timeout |
How do I relax the timeout? |
Add |
42206b6
to
f3b847c
Compare
It seems like it's still timing out for some compilers. Should I relax it even further? |
Yes just set a reasonable timeout, and/or speed up the test |
f3b847c
to
f7a04b0
Compare
Done. The ADM mass test now succeeds after <20s for the compilers that were failing before (gcc-9, gcc-11). Now the fails are from |
Proposed changes
This PR adds volume integrands for the calculation of ADM mass, ADM linear momentum, and center of mass in the XCTS system.
The primary goal of using infinite volume integrals instead of infinite surface integrals is to improve numerical accuracy. We haven't seen significant accuracy improvements yet, and we have found some analytic test cases in which the volume integrals do not give the expected results (which could be a bug in the method or an assumption being broken somewhere in these cases). Then, we'll keep using the only surface integrals in the ID parameter control for now.
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