-
Notifications
You must be signed in to change notification settings - Fork 191
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
Adds worldtube functions of time to BinaryCompactObject
#6255
Conversation
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.
And then also fix the clang-tidy
errors
domain::creators::BinaryCompactObject<false>, | ||
domain::creators::Brick, domain::creators::Cylinder, |
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.
In the BBH executable, you also need to have BCO<false>
since we use a different list there
ASSERT(not translation_map_options_.has_value(), | ||
"Translation map is not implemented for worldtube evolutions."); |
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.
Make these ERROR
s. If the user specifies an incorrect option, we should get an error and not be able to proceed
tmpl::pair<DomainCreator<volume_dim>, | ||
tmpl::list<domain::creators::BinaryCompactObject<false>>>, |
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.
Should this be <true>
? Or are you not enabling the worldtube FoTs yet?
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 am not enabling them yet but will do so in a follow-up PR (hopefully tomorrow). Currently, the worldtube fots are not being updated, so the executable would not be usable.
if constexpr (UseWorldtube) { | ||
static_assert(not IsCylindrical, | ||
"Cylindrical map not supported with worldtube"); | ||
return create_worldtube_functions_of_time(); |
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.
Also (void)initial_expiration_times
. Or you could optionally check that the initial expiration times are empty because there shouldn't be any control systems for the CSW system.
@knelli2 thanks, I included your suggestions. test failures look unrelated to me and I am not sure why clang-tidy got stuck |
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. Yeah I'm not sure why clang-tidy is being difficult. This has happened on other PRs as well.
Squashed in, thank you for your review! |
Ignoring unrelated test failures |
This PR allows the
BinaryCompactObject
domain creator to return the functions of time needed by the worldtube evolution.This required either adding a Worldtube argument that could be determined in the input file at runtime or templating the class on a bool. @knelli2 and I decided it is best to use the latter approach, as a
UseWorldtube
option in the input file might be confusing to 99 % of users who would not need it. The template parameter is defaulted tofalse
and only this instantiation is added to the Factory so it should be very hard for a user to use the wrong one by accident.