Skip to content
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

WorldtubeBufferUpdater and WorldtubeDataManager for CCE #5640

Merged

Conversation

Sizheng-Ma
Copy link
Contributor

Proposed changes

This PR aims to pave the path for dealing with the worldtube data of the Klein-Gordon CCE system. Two major changes are introduced to the current infrastructure

  1. The current WorldtubeDataManager is hard-coded to handle Tags::characteristic_worldtube_boundary_tags<Tags::BoundaryValue>. This can be relaxed so that it can be used to deal with other worldtube data (say a scalar field).
  2. There are repeated contents in WorldtubeBufferUpdater and WorldtubeDataManager, and will be more once they are extended to handle the Klein-Gordon system. I extract some common parts into functions to avoid more copy and paste.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@Sizheng-Ma
Copy link
Contributor Author

I've tested the executable CharactersiticExtract with a BBH system. All the waveform outputs are not impacted by the changes.

src/Evolution/Systems/Cce/WorldtubeBufferUpdater.hpp Outdated Show resolved Hide resolved
src/Evolution/Systems/Cce/WorldtubeBufferUpdater.hpp Outdated Show resolved Hide resolved
src/Evolution/Systems/Cce/WorldtubeDataManager.hpp Outdated Show resolved Hide resolved
src/Evolution/Systems/Cce/WorldtubeDataManager.hpp Outdated Show resolved Hide resolved
@Sizheng-Ma
Copy link
Contributor Author

Done!

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Squash all the fixups including these last few changes.

src/Evolution/Systems/Cce/WorldtubeDataManager.cpp Outdated Show resolved Hide resolved
src/Evolution/Systems/Cce/WorldtubeDataManager.cpp Outdated Show resolved Hide resolved
@Sizheng-Ma Sizheng-Ma force-pushed the worldtube_manager_updater branch 2 times, most recently from 3fac882 to 85e7d6e Compare November 28, 2023 13:35
@Sizheng-Ma
Copy link
Contributor Author

Done. I noticed that the first argument of set_time_buffer_and_lmax was also a reference. I switched it to gsl::not_null in the last commit.

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. You can squash the final fixup

@Sizheng-Ma
Copy link
Contributor Author

Ok, squashed.

@knelli2 knelli2 merged commit 3e7e020 into sxs-collaboration:develop Dec 5, 2023
20 of 22 checks passed
@Sizheng-Ma Sizheng-Ma deleted the worldtube_manager_updater branch December 5, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants