Log average values

[paulf81]

Hercules features an input value log_every_n that, to save space and make post-processing more efficient, if > 1, logs every nth step to the log file. Previously, this worked exactly as said, if log_every_n was 4, and only every 4th step would be logged and the time step of the log file would be that multiple of the underlying time step, ie:

self.dt_log = self.dt * self.log_every_n

This PR mostly keeps that same behavior, but instead of logging every nth value, the average over those n-values is logged (except for step, time and time_utc which are left to be the first step in the n_window.)

The reason for the change is that there can be be minor overshoots in power during simulation, that are corrected in one step. However, if we log say only every 10th step, these overshoots can appear to last 10 steps, which can meaningfully distort the results. This PR corrects by using a simple arithmatic mean in its place to restore the overshoot to a more minor issue.

[Copilot]

🟡 Changes recommended

It silently ignores explicitly requested reserved external signal names (time, time_utc) in external_data.log_channels, which should fail fast to avoid confusing mismatches in output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR changes Hercules’ log_every_n behavior from downsampling (logging every Nth step) to logging window-averaged values over each N-step window, reducing aliasing artifacts from transient overshoots while keeping the same row cadence (dt_log = dt_sim * log_every_n).

Changes:

• Update HDF5 logging to accumulate per-step values and write per-window arithmetic means, while keeping time/step anchored to the first step in each window.
• Add metadata.logging_mode = "window_average" and clarify logging semantics across docs.
• Strengthen tests to deterministically validate window-averaged logging (including a short final window).

File summaries

File	Description
hercules/hercules_model.py	Implements window-averaged logging, adds logging mode metadata, and skips reserved external-signal timing columns.
tests/hercules_model_test.py	Updates log_every_n tests to verify window averaging using controlled plant.power values.
docs/output_files.md	Documents new metadata semantics (logging_mode) and updated meaning of log_every_n.
docs/hercules_model.md	Explains window-averaged behavior and its effect on time/step/time_utc.
docs/hercules_input.md	Updates user-facing input documentation for log_every_n window averaging.
docs/h_dict.md	Updates log_every_n description to reflect window-averaged meaning.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[misi9170]

Instead of changing what log_every_n does, could we have options for how to aggregate? I believe such a set up is common in databases. This would look something like:

log_every_n: 10
aggregation_method: "mean" # or, "last", "first", "max", "min", "median" (?), "instant" (?), "count", "sum", ...

It would seem to me that hardcoding the mean is a somewhat significant change in the behavior, although I suppose in most (maybe all?) of the examples we just log every 1, so those wouldn't change.

[paulf81]

Instead of changing what log_every_n does, could we have options for how to aggregate? I believe such a set up is common in databases. This would look something like:

log_every_n: 10
aggregation_method: "mean" # or, "last", "first", "max", "min", "median" (?), "instant" (?), "count", "sum", ...

It would seem to me that hardcoding the mean is a somewhat significant change in the behavior, although I suppose in most (maybe all?) of the examples we just log every 1, so those wouldn't change.

That makes sense @misi9170 , so if I code so we keep log_every_n and add an input for aggregation method (that defaults to mean if not present for backwards compatiblity, albeit a change (for the better!) in behavior), does that work you think?

[misi9170]

@paulf81 or have the default be "first" (or maybe "last") for complete backwards compatibility? Unless you feel strongly that "mean" is a better default (I'm not so sure about that, honestly---the "first" or "last" seem to me the most explicit methods of downsampling)

[paulf81]

I had agreed on first being the most intuitive, but I think in the case of Hercules/Hycon, this specific behavior of minor overshoots getting exaggerated in the process (a 1-s overshoot made to look like a 60s overshoot) makes me think mean is the safer default behavior

[misi9170]

Hmm... might just have to agree to disagree. I see what you mean, but it seems to me that an overshoot could be catastrophic, so shouldn't be "hidden" in a mean unless the user specifically asks for it. But then again, sampling every n can also easily miss overshoots, so perhaps the counterargument is that at least the mean captures all data. The most conservative is probably max, or maybe min, but those are not very good defaults :)

[paulf81]

Ok @misi9170 , I take your point here. The overshoot issue is I understand is mostly an artifact of larger time steps, we don't want the controller to cheat, so the true solution would be smaller time steps, but that mostly costs a lot more for a little benefit. Maybe let's just talk this through live tomorrow if you have time, I agree this one is probably worth not rushing a decision on