-
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
Boundary component for Klein-Gordon CCE #5683
Boundary component for Klein-Gordon CCE #5683
Conversation
typename BoundaryCommunicationTagsList1, | ||
typename... BoundaryCommunicationTagsList2> |
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.
With just this commit, this actually adds a Variables tag with an empty list since there are no extra tag lists yet (which is not something we want). I'd recommend something like
template <typename Initializer, typename ManagerTags,
typename... BoundaryCommunicationTagsList> {
// other stuff
simple_tags = tmpl::list<::Tags::Variables<BoundaryCommunicationTagsList>...>;
// inside apply function
Initialization::mutate_assign<simple_tags>(make_not_null(&box),
Variables<BoundaryCommunicationTagsList>{
Spectral::Swsh::number_of_swsh_collocation_points(l_max)}...);
};
Not that we'll likely need more than 2 lists, but this also avoids the annoying 1
and 2
suffixes.
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.
Good idea!
CceComputationTestHelpers.cpp | ||
KleinGordonBoundaryTestHelpers.cpp |
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 to add the hpp
too.
* - initialization tag | ||
* `Cce::Tags::H5WorldtubeBoundaryDataManager`, | ||
* `Cce::Tags::KleinGordonH5WorldtubeBoundaryDataManager`. |
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.
We don't have the initialization_tag
type alias anymore, it's called simple_tags_from_options
now. I know it's incorrect for the other ones as well. Could you please fix those dox in a separate commit?
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.
Done!
4780b66
to
bd4de36
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
d3ae3b1
to
68b9b20
Compare
Done! |
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.
Test timeouts unrelated.
Proposed changes
This PR adds a boundary parallel component
KleinGordonH5WorldtubeBoundary
for Klein-Gordon CCE. The component has a single initialization actionActions::InitializeWorldtubeBoundary<KleinGordonH5WorldtubeBoundary<Metavariables>>
, which initializes two tag listsMetavariables::cce_boundary_communication_tags
andMetavariables::klein_gordon_boundary_communication_tags
( the metavariable will be added in the next PR). The third and fourth commits of this PR add the initialization action and the component respectively.The initialization action is constructed from a base class
InitializeWorldtubeBoundaryBase
, which currently holds a single tag list. This is fine for the current CCE system since the code treatsMetavariables::cce_boundary_communication_tags
as one::Tags::Variables
. However, for the upcoming Klein-Gordon CCE system, we want to handleMetavariables::cce_boundary_communication_tags
andMetavariables::klein_gordon_boundary_communication_tags
separately, so we initialize them as two different chunks of::Tags::Variables
. In the first commit, a tag pack is added toInitializeWorldtubeBoundaryBase
for this purpose.The second commit is a minor change. The function
create_fake_time_varying_klein_gordon_data
is moved to a new file so that it can be used by other unit tests.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