Use DampingFunctions for GH evolutions #2597
Conversation
geoffrey4444
added
the
in progress
geoffrey4444
force-pushed
the
GhDampingFunctions
branch
from
d26c1aa
to
3fbee55
Compare
geoffrey4444
removed
the
in progress
| template <size_t VolumeDim, typename Fr> | ||
| auto GaussianPlusConstant<VolumeDim, Fr>::get_clone() const noexcept | ||
| -> std::unique_ptr<DampingFunction<VolumeDim, Fr>> { | ||
| return std::make_unique<GaussianPlusConstant<VolumeDim, Fr>>( |
There was a problem hiding this comment.
If you make the class copyable (which it very easily could and should be) then you can do make_unique<...>(*this)
There was a problem hiding this comment.
I thought, like with MathFunctions, we don't want them to be copyable, so that we force users to avoid copying? But if you're cool with it, I'm fine making them copyable
There was a problem hiding this comment.
OK, I made the class (and the base class) copyable by no longer deleting the copy constructors
There was a problem hiding this comment.
Yea, but I make poor decisions just like everyone else :) When having a get_clone function it's much easier if you can copy construct 🤷
|
|
||
| static constexpr bool pass_metavariables = false; | ||
| static type create_from_options(const type& damping_function) noexcept { | ||
| return std::move(damping_function->get_clone()); |
There was a problem hiding this comment.
Does the std::move actual do anything here? You're already getting an rvalue result from get_clone
There was a problem hiding this comment.
Nope! Thanks for pointing this out, as it directed me to clear up lvalue vs rvalue. I found this page very helpful for this: https://en.cppreference.com/w/cpp/language/value_category
| @@ -17,6 +18,8 @@ spectre_target_headers( | |||
| HEADERS | |||
| GaussianPlusConstant.hpp | |||
| DampingFunction.hpp | |||
| RegisterDerivedWithCharm.hpp | |||
There was a problem hiding this comment.
Not related to the changes of this PR, but, since you are editing this file, it wouldn't hurt to fix the alphabetical order of these headers. Up to you.
There was a problem hiding this comment.
Thanks for catching this! I meant to fix this myself. Not sure how to do it, but would be great if someday this was caught automatically somehow.
| } // namespace GeneralizedHarmonic::ConstraintDamping::Tags | ||
|
|
||
| /*! | ||
| * \brief A DampingFunction to compute the constraint damping paramter |
There was a problem hiding this comment.
paramter -> parameter here and in two more occurrences below.
geoffrey4444
force-pushed
the
GhDampingFunctions
branch
from
3fbee55
to
bff9cbd
Compare
geoffrey4444
force-pushed
the
GhDampingFunctions
branch
from
bff9cbd
to
3917c04
Compare
|
@carmaza @nilsdeppe thanks for the very fast reviews! Very much appreciated :) I made the changes you suggested and squashed |
Proposed changes
For the generalized harmonic system, this PR replaces hard-coded constraint damping parameter functions with DampingFunctions whose parameters are specified in the yaml input file. The value of the parameters in the unit tests and in KerrSchild.yaml are the same as the previously hard-coded values, which originally came from SpEC input files for a single black hole.
Since the constraint damping functions now live in a namespace GeneralizedHarmonic::ConstraintDamping, this PR also moves the other tags relevant to constraint damping parameters from GeneralizedHarmonic into GeneralizedHarmonic::ConstraintDamping.
Note: the DampingFunctions currently are constant in time. A future PR will make DampingFunctions aware of FunctionsOfTime, so that, e.g., you can scale the damping parameters by the expansion factor (as SpEC does for binary black hole mergers).
Types of changes:
Component:
Code review checklist
clang-tidy.For instructions on how to perform the CI checks locally refer to the Dev
guide on the Travis CI.
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
Further comments