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.

Sign up for GitHub

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

Jump to bottom

Change ReduceCceWorldtube to use options #6287

Merged
merged 2 commits into from
Oct 5, 2024
Merged

Change ReduceCceWorldtube to use options #6287

merged 2 commits into from
Oct 5, 2024

Conversation

knelli2
Copy link
Contributor

@knelli2 knelli2 commented Sep 12, 2024

Proposed changes

Towards #6246.

Since there will be a lot more functionality added to the ReduceCceWorldtube exec (combining h5 files, dealing with overlapping times, handling nodal data), it will be easier to add this if we use some kind of Option framework. This PR keeps the same functionality as before, but just moves all the options from the command line to a YAML.

Upgrade instructions

ReduceCceWorldtube executable no longer accepts command-line arguments. They must now be specified in a YAML file. See src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.yaml for an example. You can then run the executable similar to our charm executables as

ReduceCceWorldtube --input-file ReduceCceWorldtube.yaml

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

@knelli2
Copy link
Contributor Author

knelli2 commented Sep 12, 2024

@markscheel (maybe @keefemitman?) Do we even do this anymore in SpEC? Are any modern runs using the old metric format? I would ideally like to test this out to ensure I didn't break anything, but I'm not sure where to get the data to try.

@knelli2 knelli2 force-pushed the reduce_cce_options branch 3 times, most recently from a0a4ecd to d5cadd3 Compare September 13, 2024 00:26
nilsdeppe
nilsdeppe requested changes Oct 1, 2024
View reviewed changes
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor suggestions. Please squash immediately!

cmake/AddInputFileTests.cmake
@@ -109,13 +109,20 @@ function(add_single_input_file_test INPUT_FILE EXECUTABLE COMMAND_LINE_ARGS
endfunction()

# Searches the directory INPUT_FILE_DIR for .yaml files and adds a test for each
# one. See `WritingTests.md` for details on controlling input file tests.
function(add_input_file_tests INPUT_FILE_DIR)
# one. See `WritingTests.md` for details on controlling input file tests. Add
Copy link
Member

Choose a reason for hiding this comment

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

Does the WritingTests.md still need to be updated about the whitelist here?

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
@@ -275,6 +282,107 @@ void perform_cce_worldtube_reduction(
}
Parallel::printf("\n");
}

namespace OptionTags {
struct InputSpecH5File {
Copy link
Member

Choose a reason for hiding this comment

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

I'd say let's get rid of references to SpEC already. InputH5File for the name :)

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
struct InputSpecH5File {
using type = std::string;
static constexpr Options::String help =
"Name of SpEC H5 worldtube file. A '.h5' extension will be added if "
Copy link
Member

Choose a reason for hiding this comment

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

Name of the H5 worldtube file. ...

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
static constexpr Options::String help =
"Apply corrections associated with documented SpEC worldtube file "
"errors. If you are using worldtube data from SpECTRE or from another NR "
"code but in the SpECTRE format, then this option should be 'False'";
Copy link
Member

Choose a reason for hiding this comment

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

should -> must? Just to make it very clear it's not a know that folks should "try out"?

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
static constexpr Options::String help =
"Number of time steps to load during each call to the file-accessing "
"routines. Higher values mean fewer, larger loads from file into RAM. "
"Default 2000.";
Copy link
Member

Choose a reason for hiding this comment

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

Default 2000. -> Set to 'Auto' to use a default value (2000).

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
"the boundary computations will be performed at a resolution that is "
"lmax_factor times the input file lmax to avoid aliasing");
"input-file", boost::program_options::value<std::string>()->required(),
"Name of YAML file to parse.");
Copy link
Member

Choose a reason for hiding this comment

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

file to parse -> input file to use.

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
Parallel::printf("%s\n", desc);
// Option parser for all the actual options
Options::Parser<option_tags> parser{
"This executable is used for converting the unnecessarily large SpEC "
Copy link
Member

Choose a reason for hiding this comment

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

unnecessarily large SpEC -> metric

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp Outdated
// Option parser for all the actual options
Options::Parser<option_tags> parser{
"This executable is used for converting the unnecessarily large SpEC "
"worldtube data format into a far smaller representation (roughly a "
Copy link
Member

Choose a reason for hiding this comment

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

far -> remove

src/Executables/ReduceCceWorldtube/ReduceCceWorldtube.cpp
tuples::get<ReduceCceTags::FixSpecNormalization>(inputs));
} catch (const std::exception& exception) {
Parallel::printf("%s\n", exception.what());
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

return 2;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually use 2 in our charm execs to mean "continue from checkpoint". src/Parallel/ExitCode.hpp Should I still use that here?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, hmm, I was just thinking a number to distinguish from the other error. Maybe we should choose and document a number for "Input file parse failed" and then use 1 as " 🤷 it went bad"

I'm happy to defer this until some other point. I think it's beyond the scope of this PR. Up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like dedicating another number to "input file parse failed" and using 1 for general error. But yeah it's outside the scope of this PR, so I'll keep it as 1 for now and change it later.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

tests/InputFiles/ReduceCceWorldtube/ReduceCceWorldtube.yaml Outdated

# [reduce_cce_worldtube_yaml_doxygen_example]

InputSpecH5File: SpecFilenameR0292.h5
Copy link
Member

Choose a reason for hiding this comment

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

Spec -> Input? Unless we actually have this file in the repo for testing, then leave as-is

@knelli2 knelli2 force-pushed the reduce_cce_options branch 2 times, most recently from 0de17da to 58586af Compare October 2, 2024 18:52
@knelli2
Copy link
Contributor Author

knelli2 commented Oct 2, 2024

@nilsdeppe Squashed in the changes into the first commit. I added a second commit that reorganizes the CCE tarball like so:

CceExecutables.tar.xz:

  • CharacteristicExtract
  • CharacteristicExtract.yaml
  • ReduceCceWorldube/
    • ReduceCceWorldtube
    • ReduceCceWorldtube.yaml
  • Tests/
    • H5 files
    • CheckCceOutput.py

I had to update the google drive link for CheckCceOutput.py. I left the old one just in case.

nilsdeppe
nilsdeppe previously approved these changes Oct 3, 2024
View reviewed changes
@knelli2 knelli2 added the in progress Don't review, used for sharing code and getting feedback label Oct 4, 2024
@knelli2
Change ReduceCceWorldtube to use options
adb7afd
@knelli2
Reorganize CCE static exec tarball
e17305c 
This will require us to edit the python file in the google drive
@knelli2 knelli2 removed the in progress Don't review, used for sharing code and getting feedback label Oct 4, 2024
@knelli2
Copy link
Contributor Author

knelli2 commented Oct 4, 2024

@nilsdeppe Ok I tested this on my fork by creating a release, and the DeployStaticExecs workflow ran successfully. I also downloaded that new tarball and ran it myself and it worked. So it's all good now!

@nilsdeppe nilsdeppe merged commit 298623c into sxs-collaboration:develop Oct 5, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilsdeppe nilsdeppe nilsdeppe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@knelli2 @nilsdeppe