Add reduced_diags general input #5479
Conversation
There was a problem hiding this comment.
Unfortunately, I think ubuntu-23.10 is not an available runner: https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories. This might explain why the CI jobs are not starting.
I'm trying to remove the container and use ubuntu-24.04 in #5474, but I'm finding new bugs that I haven't been able to fix yet.
There was a problem hiding this comment.
@dpgrote I guess the changes to this file can be reverted now that the sanitizer tests have been fixed / disabled.
There was a problem hiding this comment.
Good point about adding this to picmi. So it doesn't get forgotten, I'll add it to this PR. And I'll undo the change in the workflows script.
There was a problem hiding this comment.
Though, looking at the picmi interface, this doesn't really apply there since each diagnostic is setup separately. There isn't a clear way to set a parameter for all reduced diagnostics. Do you have any suggestions?
There was a problem hiding this comment.
One option would be to add arguments to Simulation that set global diagnostic parameters (period and output directory, etc.). These parameters could then be passed along to diagnostic_initialize_inputs() to be used if the individual diagnostic did not have a value set. It will require changing some defaults, since currently the default period for reduced diagnostics is 1, but I guess we'd have to change it to None in order to check if a value was set.
There was a problem hiding this comment.
Ok, that could work. It can be a bit simpler too since Simulation could directly set the parameters, e.g. reduced_diags.intervals.
BTW, the default for the period in ReducedDiagnostic should be None anyway so that it doesn't override the default set in C++.
for more information, see https://pre-commit.ci
| reduced_diags = pywarpx.warpx.get_bucket("reduced_diags") | ||
| reduced_diags.path = self.reduced_diags_path | ||
| reduced_diags.extension = self.reduced_diags_extension | ||
| reduced_diags.intervals = self.reduced_diags_intervals | ||
| reduced_diags.separator = self.reduced_diags_separator | ||
| reduced_diags.precision = self.reduced_diags_precision |
There was a problem hiding this comment.
Nice! This is even cleaner than my original suggestion 🎉
This PR adds the
reduced_diagsinput parameter group that allows setting of parameters common to all reduced diagnostics. For example 'reduced_diags.intervals` can be set once rather than having to set it for each individual reduced diagnostic.