-
Notifications
You must be signed in to change notification settings - Fork 184
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
Write an H5::Cce file from the *CharacteristicExtract execs #5985
Conversation
Note that I still need to update the CCE tutorial docs |
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.
Ok @nilsdeppe this is ready for review
|
||
const std::string subfile_name{"Cce"}; |
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.
Is this what we want to name the subfile? This produces output in the H5 file like
/Cce.cce Group
/Cce.cce/EthInertialRetardedTime Dataset {26451/Inf, 163}
/Cce.cce/News Dataset {26451/Inf, 163}
/Cce.cce/Psi0 Dataset {26451/Inf, 163}
/Cce.cce/Psi1 Dataset {26451/Inf, 163}
/Cce.cce/Psi2 Dataset {26451/Inf, 163}
/Cce.cce/Psi3 Dataset {26451/Inf, 163}
/Cce.cce/Psi4 Dataset {26451/Inf, 163}
/Cce.cce/Strain Dataset {26451/Inf, 163}
/src.tar.gz Dataset {7757329}
which seems a bit redundant? Other options could be /Bondi.cce
, /Scri.cce
, /FutureNullInfinity.cce
, /SpECTRE.cce
.
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 agree. I think any are fine, except maybe Bondi
to avoid confusion with the BondiSachs
worldtube data we write out from GH that's input into CCE? Another option could be to append the worldtube radius? E.g. SpectreR100.cce
in case we do want to run multiple CCE's simultaneously in the future.
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.
LGTM, please go ahead and squash :)
|
||
const std::string subfile_name{"Cce"}; |
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 agree. I think any are fine, except maybe Bondi
to avoid confusion with the BondiSachs
worldtube data we write out from GH that's input into CCE? Another option could be to append the worldtube radius? E.g. SpectreR100.cce
in case we do want to run multiple CCE's simultaneously in the future.
bbdbcd5
to
0cafb3e
Compare
@nilsdeppe Rebased and squashed in existing fixups. Added one new commit that adds the extraction radius to the global cache for CCE execs because it wasn't there before, and then another fixup that changes the subfile name to |
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 great! A couple small optional things you can do if you want while squashing. Thanks for checking with Keefe! I like this naming :)
#include <string> | ||
|
||
namespace Cce { | ||
// retrieves the extraction radius from the specified file. |
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.
Add another slash to make these Doxygen comments?
And maybe group together with next one?
ERROR( | ||
"The CCE filename must encode the extraction radius as an integer " | ||
"between the first instance of 'R' and the first instance of '.' " | ||
"(SpEC " |
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.
[optional] Combine with next line?
Writes data to an h5::Cce subfile
Proposed changes
Instead of writing a bunch of
.dat
files in the H5 file for each Bondi quantity at future null infinity,CharacteristicExtract
andKleinGordonCharacteristicExtract
now output anH5::Cce
subfile. See #5963 for more details about the subfile.Upgrade instructions
The output from CCE in the reductions file should only be a
Cce.cce
subfile. It won't have a bunch of.dat
files anymore.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