-
Notifications
You must be signed in to change notification settings - Fork 19
Reorganize logging in Hercules #167
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
Reorganize logging in Hercules #167
Conversation
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.
Pull Request Overview
This PR refactors the logging system from a fixed logging_option parameter to a flexible log_channels configuration. Key changes include replacing logging_option ("base", "turb_subset", "all") with a comma-separated log_channels string that allows users to specify exactly which outputs to log. The PR also renames wind-related variables for consistency (e.g., velocities → wind_speeds, wind_speed → wind_speed_mean_unwaked) and adds support for selective array element logging (e.g., turbine_powers.001 to log only specific turbines).
- Replaces
logging_optionwithlog_channelsparameter across all components - Renames wind speed variables for clarity and consistency
- Adds selective array element logging feature with
.NNNsyntax - Updates all configuration files, tests, and documentation
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| hercules/plant_components/component_base.py | Adds log_channels parsing logic to base component class |
| hercules/plant_components/wind_meso_to_power.py | Removes logging_option validation, renames variables, removes hard-coded log outputs list |
| hercules/plant_components/wind_meso_to_power_precom_floris.py | Same refactoring as wind_meso_to_power.py for precomputed FLORIS variant |
| hercules/plant_components/solar_pysam_base.py | Removes log_extra_outputs logic in favor of log_channels |
| hercules/plant_components/battery_simple.py | Removes hard-coded log outputs list |
| hercules/plant_components/battery_lithium_ion.py | Removes hard-coded log outputs list |
| hercules/plant_components/electrolyzer_plant.py | Adds removal of log_channels before passing config to Supervisor |
| hercules/emulator.py | Implements selective array element logging logic in HDF5 initialization and data logging |
| tests/*.py | Updates tests to use renamed variables and new log_channels parameter |
| examples/*.yaml | Updates example configurations from logging_option to log_channels |
| docs/*.md | Updates documentation to reflect new logging system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@paulf81 , overall this looks good, although I haven't yet done a really detailed run-through. A couple of suggestions jump out at me though:
which might be a little easier both to read on the input file and handle within the |
Both great suggestions, I'll get these in! |
|
ok @misi9170 requested changes are in, and develop is merged in as well |
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.
Pull Request Overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ax.plot( | ||
| df["time"], | ||
| df["wind_farm.floris_wind_speed"], | ||
| df["wind_farm.wind_speed_mean"], |
Copilot
AI
Oct 30, 2025
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.
The column name should be wind_farm.wind_speed_mean_background to match the new naming convention used throughout the codebase. The current name wind_farm.wind_speed_mean is inconsistent with the renamed variables.
| df["wind_farm.wind_speed_mean"], | |
| df["wind_farm.wind_speed_mean_background"], |
|
|
||
| component_type: Wind_MesoToPower | ||
| floris_input_file: inputs/floris_input.yaml | ||
| wind_input_filename: inputs/wind_input.csv |
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.
This example seems to have an unrelated bug: the inputs/wind_input.csv file referred to here doesn't seem to exist
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.
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 good, thanks @paulf81 ! I left one suggestion below, but approving now anyway. I'm also happy to update if you'd like.
|
I ran various examples with different configurations both of the list |
Previously, selecting which channels to log was a little ad hoc, with options including "logging_options" and "log_extra_channels" floating around. This PR streamlines this in the following ways:
Finally, some of the channel names in the wind components were standardized to make this simpler. For example, what used to be wind_speed, is now wind_speed_mean_unwaked to make very explicit that value implied.