-
Notifications
You must be signed in to change notification settings - Fork 19
Feature/wind consolidation #172
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
Conversation
|
Replacing with #173 |
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 pull request refactors the wind farm simulation architecture by unifying three wake modeling strategies into a single WindFarm class and introducing a new WindFarmSCADAPower class for SCADA-based power simulations. The refactoring maintains backward compatibility while simplifying the codebase by consolidating previously separate classes (Wind_MesoToPower and Wind_MesoToPowerPrecomFloris) into one unified implementation.
Key changes:
- Unified wind farm implementation with configurable wake models ("dynamic", "precomputed", "no_added_wakes")
- New
WindFarmSCADAPowercomponent for SCADA power data simulations - Deprecated standalone
wind_meso_to_power_precom_floris.pymodule (functionality moved toWindFarmclass)
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
hercules/plant_components/wind_farm.py |
Unified WindFarm class with three wake modeling strategies (dynamic, precomputed, no_added_wakes) |
hercules/plant_components/wind_farm_scada_power.py |
New class for SCADA power-based simulations |
hercules/plant_components/wind_meso_to_power_precom_floris.py |
Removed (functionality consolidated into WindFarm) |
hercules/hybrid_plant.py |
Updated to use new unified classes with wake model mapping |
hercules/utilities.py |
Added Wind_MesoToPowerNoAddedWakes to available component types |
tests/wind_meso_to_power_test.py |
Updated tests to use WindFarm instead of Wind_MesoToPower |
tests/wind_meso_to_power_precom_floris_test.py |
Updated tests to use WindFarm instead of Wind_MesoToPowerPrecomFloris |
tests/wind_farm_scada_power_test.py |
New comprehensive test suite for WindFarmSCADAPower |
tests/wind_farm_direct_test.py |
New test suite for direct wake mode (no_added_wakes) |
tests/test_inputs/scada_input.csv |
New test input data for SCADA power tests |
examples/02c_wind_farm_realistic_inflow_direct/* |
New example demonstrating direct wake model usage |
docs/wind.md |
Updated documentation for all four wind farm components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "wind_farm": [ | ||
| "Wind_MesoToPower", | ||
| "Wind_MesoToPowerPrecomFloris", | ||
| "Wind_MesoToPowerNoAddedWakes", |
Copilot
AI
Nov 5, 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 available component types list is missing 'WindFarmSCADAPower', which is a new component type introduced in this PR. This should be added to the list to ensure it's recognized as a valid component type.
| "Wind_MesoToPowerNoAddedWakes", | |
| "Wind_MesoToPowerNoAddedWakes", | |
| "WindFarmSCADAPower", |
| df_scada["time_utc"] = pd.to_datetime(df_scada["time_utc"], utc=True) | ||
|
|
||
| # Ensure time_utc is timezone-aware (UTC) | ||
| if not isinstance(df_scada["time_utc"].dtype, pd.DatetimeTZDtype): |
Copilot
AI
Nov 5, 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.
This uses isinstance(df_scada['time_utc'].dtype, pd.DatetimeTZDtype) which differs from the pattern used in wind_farm.py line 124 (same check). For consistency, both files should use the same approach. The original code in wind_meso_to_power_precom_floris.py used pd.api.types.is_datetime64tz_dtype(), which is more commonly used and explicit. Consider using the same pattern across both files for maintainability.
| if not isinstance(df_scada["time_utc"].dtype, pd.DatetimeTZDtype): | |
| if not pd.api.types.is_datetime64tz_dtype(df_scada["time_utc"]): |
|
|
||
| Args: | ||
| dt (float): Time step for the simulation in seconds. | ||
| initial_scada_powers (np.ndarray): Initial wind speeds in m/s for all turbines |
Copilot
AI
Nov 5, 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 docstring incorrectly describes initial_scada_powers as 'Initial wind speeds in m/s for all turbines'. This parameter actually represents initial SCADA power values in kW, not wind speeds. The docstring should read 'Initial SCADA power values in kW for all turbines to initialize the simulation.'
| initial_scada_powers (np.ndarray): Initial wind speeds in m/s for all turbines | |
| initial_scada_powers (np.ndarray): Initial SCADA power values in kW for all turbines |
| response to changing wind conditions. | ||
|
|
||
| Args: | ||
| scada_powers (np.ndarray): Current SCADA powers for all turbines. |
Copilot
AI
Nov 5, 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 docstring for the scada_powers parameter should specify the units (kW) for consistency with other parameter descriptions in the codebase. It should read 'Current SCADA powers in kW for all turbines.'
| scada_powers (np.ndarray): Current SCADA powers for all turbines. | |
| scada_powers (np.ndarray): Current SCADA powers in kW for all turbines. |
| - wind_speeds_withwakes | ||
| - wind_speeds_background | ||
| - turbine_power_setpoints | ||
| floris_update_time_s: 300.0 # Not used but kept for interface consistency |
Copilot
AI
Nov 5, 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.
[nitpick] Requiring floris_update_time_s for the Wind_MesoToPowerNoAddedWakes component type when it's explicitly not used creates unnecessary configuration burden. Consider making this parameter optional for wake models that don't use it, or document in the code why this design decision was made for interface consistency.
| floris_update_time_s: 300.0 # Not used but kept for interface consistency | |
| # Note: floris_update_time_s is not required for Wind_MesoToPowerNoAddedWakes and is intentionally omitted. |
This pull request reorganizes the wind farm simulation component in a few ways:
Combines what had been two separate classes (
Wind_MesoToPowerandWind_MesoToPowerPrecomFloris) into one (WindFarm) class to capitalize on a lot of repeated code outside the wake modelingAdds two new component types:
a)
Wind_MesoToPowerNoAddedWakesuses the wind speeds in the input without adding wakes (added as 3rd option toWindFarm)b)
WindFarmSCADAPowerimplemented as new class because more substantially different, passes through power in SCADA data but does allow limiting by setpoints (although worth noting power in SCADA may already be de-rated)Besides that adds relavent tests, examples and docs