Add features to history output #1024
Conversation
brryan
changed the title
| @@ -147,9 +149,20 @@ struct HistoryOutputVar { | |||
| : hst_op(hst_op_), hst_fun(hst_fun_), label(label_) {} | |||
| }; | |||
|
|
|||
| struct HistoryOutputVec { | |||
There was a problem hiding this comment.
I treated vector output separately from scalar history output because I didn't see another way to do this apart from a breaking change where enrolled history functions only return std::vector<Real> i.e. all existing history implementations would need to switch.
| for (const auto &pkg_name : requested_packages) { | ||
| const auto &it = all_packages.find(pkg_name); | ||
| if (it != all_packages.end()) { | ||
| packages[(*it).first] = (*it).second; |
There was a problem hiding this comment.
Should we throw an error if the packages has not been found? To prevent accidental typos in the input file.
There was a problem hiding this comment.
Yeah I agree. One could imagine someone wanting to write out history from specific packages and automatically not getting that output if they enable/disable individual packages at runtime, but I think catching typos is more important. Also, a way to disable packages at runtime is to enroll an empty package with the same name, which would work for the above.
Changed to error if this if statement is false.
| ["power0", 7.06177e-02, 1.39160e-02], | ||
| ["power1", 3.88112e-02, 2.59597e-03], | ||
| ["power2", 2.65948e-02, 7.19427e-04], |
There was a problem hiding this comment.
This should be advected_powers#, isn't it?
Also, if there are no objection, I'd prefer the indices separated by _ as we're using that kind of convention in other places already.
There was a problem hiding this comment.
Yes, the other labels were actually wrong as well (e.g. total should have been total_advected). I changed all the variable names to be corrected and also added a test of the column headers.
And I also added the _ by default for vector-length history output so it is now e.g. advected_powers_2.
IN ADDITION I also enforced a space between each column so we can parse the labels more easily. In develop currently we can get headers like # [0]=time [1]=dt [2]=my_long_var_name[3]=my_other_longvar_name which can be annoying to parse via something simple like .split() in python. I don't think this is a breaking change because whitespace between variable names was already a possibility (for short variable names) but I should mention it.
|
@pgrete @Yurlungur @pdmullen My plan is to set this to automerge sometime tomorrow -- last call for objections! Thanks for your reviews all |
| <parthenon/output5> | ||
| file_type = ascent | ||
| dt = -0.05 # soft disabled by default, as Ascent is an optional dependency |
There was a problem hiding this comment.
I think this is what's causing the failed regression test as ascent outputs are manually enabled in .github/workflows/ci-extended.yml via parthenon/output4/dt=0.05
There was a problem hiding this comment.
🚨 thank you for noticing this @pgrete!
I also modified .github/workflows/ci-extended.yml to give an error message if test fails.
PR Summary
We want additional control in how history outputs are produced. This PR:
my_output.hsttomy_output.out2.hst]PR Checklist