-
Notifications
You must be signed in to change notification settings - Fork 19
Reconfigure wind models #173
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
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 wind farm simulation architecture by consolidating multiple wind farm model classes into a unified WindFarm class that supports three wake modeling strategies: dynamic, precomputed, and no_added_wakes. Additionally, it introduces a new WindFarmSCADAPower component that uses SCADA power measurements directly.
Key changes:
- Unified
Wind_MesoToPowerandWind_MesoToPowerPrecomFlorisinto a singleWindFarmclass with configurablewake_modelparameter - Added support for
Wind_MesoToPowerNoAddedWakesmode (no wake modeling) - Introduced
WindFarmSCADAPowercomponent for SCADA-based power simulation - Updated test files to use the unified
WindFarmclass - Removed redundant code from
wind_meso_to_power_precom_floris.py(now deleted)
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/wind_meso_to_power_test.py | Updated tests to import and use WindFarm class instead of Wind_MesoToPower |
| tests/wind_meso_to_power_precom_floris_test.py | Updated tests to import and use WindFarm class instead of Wind_MesoToPowerPrecomFloris |
| tests/wind_farm_scada_power_test.py | New comprehensive test suite for WindFarmSCADAPower component |
| tests/wind_farm_direct_test.py | New test suite for direct wake mode (Wind_MesoToPowerNoAddedWakes) |
| tests/test_inputs/scada_input.csv | New test data file for SCADA power tests |
| hercules/utilities.py | Added Wind_MesoToPowerNoAddedWakes to available component types |
| hercules/plant_components/wind_meso_to_power_precom_floris.py | Deleted (functionality moved to WindFarm) |
| hercules/plant_components/wind_farm_scada_power.py | New component for SCADA-based power simulation |
| hercules/plant_components/wind_farm.py | Unified wind farm class supporting multiple wake models |
| hercules/hybrid_plant.py | Updated to use unified WindFarm and new WindFarmSCADAPower classes |
| examples/02c_wind_farm_realistic_inflow_direct/* | New example demonstrating direct wake model |
| docs/wind.md | Updated documentation covering all four wind farm component types |
Comments suppressed due to low confidence (1)
hercules/plant_components/wind_farm.py:124
- Using
isinstance()withpd.DatetimeTZDtypeis correct for pandas 2.0+. However, for backward compatibility with pandas <2.0, consider usingpd.api.types.is_datetime64tz_dtype()as it was in the original code. The same applies to line 75 inwind_farm_scada_power.py.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…hercules into feature/wind_consolidation
|
@misi9170 and @genevievestarke ok I think this is caught up. You can see ruff is failing out but this is largely on other files. I started another PR to deal with that #191 so maybe we do that one first and then merge back one more time. |
|
Thanks @paulf81 ! I'll get this reviewed shortly, possibly Monday. I'm not sure I totally follow on the formatting---from what I see, #191 does not actually contain any formatting changes and the 6 files that are flagged in the checks here are all ones that have been modified in this PR. Possibly just something weird to do with when the branches were made? In any case, I think that they should be cleared up by just modifying a few files already changed in this PR. I can check that quickly now. |
Ah, quite possibly I got myself confused. I was thinking I was seeing format changing files that would not have been otherwise changed so forced those changes into #191. In any case if we finish that one and merge and pull into here we'll be on right path. |
|
All good! I think we're back on track here. |
genevievestarke
left a comment
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 to me, just some general comments for future development!
examples/02c_wind_farm_realistic_inflow_direct/hercules_input.yaml
Outdated
Show resolved
Hide resolved
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.
I've left a handful of small comments throughout, but my main comment is on the architecture of the WindFarm class and its options ("dynamic", "precomputed", and "no_added_wakes"). I have a couple of concerns on this:
As far as I can tell, throughout the rest of Hercules, the "component_type" field of the input file specifies exactly which class to instantiate to model that component. The WindFarm class is different: the user provides a string that refers to a deprecated class, which is then mapped to a different string ("dynamic", "precomputed", or "no_added_wakes"), and a WindFarm object is instantiated. To me, it would be more elegant if "component_type" was simply WindFarm to match the usage in other components, and then there was a separate field "wake_model" (or "wake_model_type", or similar) on the input file that the user can set to "precomputed", etc.
Alternatively, I think that each of these "wake_model"s on WindFarm could be a class on it's own. Each has it's own instantiation method, and each has a fairly different step method. To me, an alternative would be to have an (abstract) WindFarm class that all three subclasses inherit from. Each subclass would then have a method like step_inner that the step method (defined on the abstract WindFarm) calls. This is chaining inheritance, and has it's downsides, but to me this feels like a case where an abstract parent could be appropriate. This is a stylistic preference though, so I'm not hung up on it. If this approach is taken, then the "component_type" on the input file would be listed as whatever the subclass name is (e.g. "WindFarmPrecomputedWakes"), which would also be consistent with how "component_type"` is used throughout Hercules.
examples/02c_wind_farm_realistic_inflow_direct/hercules_input.yaml
Outdated
Show resolved
Hide resolved
examples/02c_wind_farm_realistic_inflow_direct/hercules_input.yaml
Outdated
Show resolved
Hide resolved
|
After discussion with @genevievestarke , we have refactored this code to have only two distinct Within the Not addressed in this PR (possibly we should create issues?):
I have run all examples; all examples run and produce visually matching outputs to the develop branch, with the exception of the 01 example, which is currently not running on either develop or this feature branch (see #158 ) |
Replaces #172
This pull request reorganizes the wind farm simulation component in a few ways:
Combines what had been two separate classes (Wind_MesoToPower and Wind_MesoToPowerPrecomFloris) into one (WindFarm) class to capitalize on a lot of repeated code outside the wake modeling
Adds two new component types:
a) Wind_MesoToPowerNoAddedWakes uses the wind speeds in the input without adding wakes (added as 3rd option to WindFarm)
b) WindFarmSCADAPower implemented 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