Solve for psi-1 in XCTS system #5684
Conversation
nilsvu
force-pushed
the
xcts_minus_one
branch
5 times, most recently
from
24beb8d
to
765e0f6
Compare
nilsvu
force-pushed
the
xcts_minus_one
branch
2 times, most recently
from
dfb9a01
to
51b6a33
Compare
|
I can review once CI is happy :) |
nilsvu
force-pushed
the
xcts_minus_one
branch
3 times, most recently
from
de9e2f8
to
bfd9810
Compare
|
CI is mostly ok now. I don't think I can silence the clang-tidy warning. |
There was a problem hiding this comment.
LGTM, couple ideas/thoughts. Feel free to respond how you prefer (e.g., just merging is acceptable :P)
Also, fantastic job on figuring all this out and getting it to work! These are in my experience the most challenging types of bugs to fix and are very frustrating. It's a huge accomplishment and I think you should be really proud! This took a lot of hard work and perseverance! 💯
|
|
||
| namespace tt { | ||
| namespace detail { | ||
| CREATE_HAS_TYPE_ALIAS(ElementType) | ||
| CREATE_HAS_TYPE_ALIAS_V(ElementType) |
There was a problem hiding this comment.
[optional] One idea to try is [[maybe_unused]] on this line. If that fails to build, we could mark the ones in the macro as that? I'm also fine ignoring and have no strong preference.
There was a problem hiding this comment.
Added a nolint block to silence this warning (new feature since clang-format 14). There's still another warning about an infinite recursion that cannot happen, which I don't think I can silence since I don't want to nolint the general code in CachedTempBuffer.
| @@ -190,8 +190,9 @@ void negative_expansion_quantities( | |||
|
|
|||
There was a problem hiding this comment.
[optional, random line] there are a lot of divisions by the conformal factor. I'm not sure if it matters because this is a BC, but it may be possible to factor those out and do only one. Probably doesn't matter, though.
There was a problem hiding this comment.
Haven't profiled this recently, but I'll keep this in mind when doing a round of optimizations 👍 Not much use to do this blindly I think.
| @@ -61,60 +61,61 @@ void add_hamiltonian_sources( | |||
| const gsl::not_null<Scalar<DataVector>*> hamiltonian_constraint, | |||
There was a problem hiding this comment.
[musing] It might be worth revisiting this in a followup performance review of the elliptic code. It might be possible to precompute the conformal factor and then pass it around. I think this is low priority, but if you have a list somewhere to keep track of these things, maybe add this?
There was a problem hiding this comment.
Yes, will do. I have no idea if precomputing it is faster or not.
Allows Blaze expressions in square, cube etc, such as `square(datavector + 1.)`.
nilsvu
force-pushed
the
xcts_minus_one
branch
2 times, most recently
from
9baf1da
to
ccd8781
Compare
|
Thank you! |
Proposed changes
Psi-1 approaches zero at the outer boundary rather than 1, hence has a more accurate numerical derivative on the stretched outer wedges. Combined with the weak primal form of the elliptic DG scheme (#5667 and #5679) this enables BBH ID with a large outer radius and fixes #4597 (some optimizations can still be done to accelerate the solves).
Try it like this:
Upgrade instructions
In XCTS input files replace
ConformalFactorandLapseTimesConformalFactorwithConformalFactorMinusOneandLapseTimesConformalFactorMinusOnein analytic boundary conditions and error observations.Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments