FLOWERS subpackage integration#169
FLOWERS subpackage integration#169cfrontin wants to merge 179 commits intoNLRWindSystems:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates the FLOWERS package (a rapid wind farm energy simulation tool) into the Ard repository. The integration includes moving FLOWERS code into the main repository structure, updating test infrastructure to support both packages, and reorganizing example data files.
Changes:
- Added complete FLOWERS package source code and tests
- Updated test scripts to support running tests for both ard and flowers packages
- Relocated example data files from
examples/data/toexamples/ard/data/ - Updated pyproject.toml to include flowers package and add Christopher Bay as an author
Reviewed changes
Copilot reviewed 35 out of 74 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| test/run_local_test_unit.sh | Updated to support running unit tests for both ard and flowers packages |
| test/run_local_test_system.sh | Updated to support running system tests for both ard and flowers packages |
| flowers/*.py | New FLOWERS source code for wind farm modeling and optimization |
| test/flowers/**/*.py | New comprehensive test suite for FLOWERS functionality |
| pyproject.toml | Updated to include flowers package in build configuration |
| examples/ard/data/*.yaml | Relocated data files with updated paths in dependent test files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…aybe more examples
jaredthomas68
left a comment
jaredthomas68
left a comment
There was a problem hiding this comment.
I'm excited to see this come in!
Most of my comments are on doc strings being out of date or missing. Should be pretty easy to fix. There are a few slightly more involved comments. I would also like to move away from using binary files and pyrite in tests if we can as I think they complicate things unnecessarily.
It looks like most of the FLOWERS tests are integration tests. Probably my biggest concern is the lack of unit testing in FLOWERS (I know you added some). Maybe breaking up the code into more methods and unit testing those needs to be in a follow on PR, but if so I think we should make an issue so we don't lost track of it.
There was a problem hiding this comment.
Non-blocking, but in the future I think we should shoot to have all resource data in the repo in the windio format
| print("Factor of Improvement: {:.1f}x".format(time_floris / time_flowers)) | ||
| print("============================") | ||
|
|
||
| if timer: |
There was a problem hiding this comment.
I have found varying returns a dangerous practice in the past. I would like to adjust this, but am up for leaving that for a later PR.
| """ | ||
|
|
||
| def __init__( | ||
| self, |
There was a problem hiding this comment.
Would be nice to use a more self-documenting approach for this init, but I think we can role with it for now
|
|
||
| if conventional_model is None or conventional_model == "gauss": | ||
| self.floris_interface = wfct.floris_interface.FlorisInterface( | ||
| "./input/gauss.yaml" |
There was a problem hiding this comment.
importing hard-coded paths this deep in the code seems dangerous to me. Perhaps we can at least make these paths using Path so they are OS agnostic. Better would be to have default dictionaries or something that will always work but could be modified by user input if desired.
| ) | ||
| elif conventional_model == "jensen": | ||
| self.floris_interface = wfct.floris_interface.FlorisInterface( | ||
| "./input/jensen.yaml" |
There was a problem hiding this comment.
See above comment about hard-coded paths
There was a problem hiding this comment.
Did you intend to include this file? If so, maybe add some comments at the top about why it is included in the repo
There was a problem hiding this comment.
are all the pickle files necessary? I would prefer to not include any pickle files for testing, but lets for sure not include ones we don't need.
|
|
||
| flowers/visualization.py | ||
| flowers/optimization/optimization_interface.py | ||
| flowers/optimization/model_interface.py No newline at end of file |
There was a problem hiding this comment.
why is model_interface.py omitted?
| @@ -23,6 +23,7 @@ authors = [ | |||
| {name = "Cory Frontin", email = "cory.frontin@nrel.gov"}, | |||
| {name = "Rafael Mudafort", email = "rafael.mudafort@nrel.gov"}, | |||
| {name = "Jared Thomas", email = "jared.thomas@nrel.gov"}, | |||
There was a problem hiding this comment.
should @locascio-m be included here?, maybe with a FLOWERS specific comment?
There was a problem hiding this comment.
or we could add an author block in the flowers pyproject file
| ] | ||
| [project.urls] | ||
| # Homepage = "https://example.com" | ||
| Documentation = "https://wisdem.github.io/flowers" |
4 participants
FLOWERS subpackage integration
While working with some offshore work, we brought in FLOWERS but in a mostly untested standalone repo; this is a more permanent home for FLOWERS in Ard that will allow it to live with Ard but still be used independently, i.e. this still would work with complete backward compatibility:
Merge with
flowersrepoThe FLOWERS git history has been preserved in its entirety in this Ard pr. Try
git blameon OG FLOWERS features to see changes frombaycorlocascio-mTo do this, I:unitandsystemsubdirectories were underneath their target subpackage in the Ard repotest/ard/unit, etc.test/flowers/systempyproject.tomlto allow independent subpackage installationpip install ardresults in bothimport flowersandimport ardbeing available in the python envOther updates
Outstanding work
File changes summarized
The following is a summary of changes by type, to help streamline the review process since this PR is so heavy.
examples/flowers/_archive/compute_AEP.py: moved to a directory for legacy testsexamples/flowers/data/HKW_wind_rose.csv: moved to a shared locationflowers/__init__.py: auto-removed trailing whitespaceflowers/flowers_model.pyflowers/optimization/__init__.pyflowers/optimization/model_interface.pyflowers/optimization/optimization_interface.pyflowers/tools.pyflowers/visualization.pypyproject_flowers.toml: renamed to dodge conflict, saved for validationtest/flowers/__init__.pytest/flowers/_archive/conftest.pytest/flowers/_archive/regression/test_flowers_aep.py.coveragerc: add ignores for legacy FLOWERS code I don't want to dispose yet.github/workflows/python-tests-consolidated.yaml: integrate parallel FLOWERS testingpyproject.toml: updated to include subpackagetest/run_local_test_system.shtest/run_local_test_unit.shexamples/{ => ard}/01_onshore/inputs/ard_system.yamlexamples/{ => ard}/01_onshore/inputs/windio.yamlexamples/{ => ard}/01_onshore/optimization_demo.ipynbexamples/{ => ard}/02_offshore_fixed/inputs/ard_system.yamlexamples/{ => ard}/02_offshore_fixed/inputs/windio.yamlexamples/{ => ard}/02_offshore_fixed/optimization_demo.ipynbexamples/{ => ard}/03_offshore_floating_custom_system/inputs/ard_system.yamlexamples/{ => ard}/03_offshore_floating_custom_system/inputs/windio.yamlexamples/{ => ard}/03_offshore_floating_custom_system/optimization_demo.ipynbexamples/{ => ard}/05_onshore_batch/inputs/NREL_Reference_5MW_126.csvexamples/{ => ard}/05_onshore_batch/inputs/ard_system.yamlexamples/{ => ard}/05_onshore_batch/inputs/h2i/driver_config.yamlexamples/{ => ard}/05_onshore_batch/inputs/h2i/h2i_ard.yamlexamples/{ => ard}/05_onshore_batch/inputs/h2i/plant_config.yamlexamples/{ => ard}/05_onshore_batch/inputs/open-meteo-56.20N8.54E86m-short.yamlexamples/{ => ard}/05_onshore_batch/inputs/open-meteo-56.20N8.54E86m.yaml.../{ => ard}/05_onshore_batch/inputs/power_thrust_table_ccblade_NREL-5p0-126-RWT.csvexamples/{ => ard}/05_onshore_batch/inputs/resource_prep/convert_weather_to_speed_dir.pyexamples/{ => ard}/05_onshore_batch/inputs/resource_prep/open-meteo-56.20N8.54E86m.csvexamples/{ => ard}/05_onshore_batch/inputs/windIO-plant_turbine_NREL-5.0MW-126m-RWT.yamlexamples/{ => ard}/05_onshore_batch/inputs/windio.yamlexamples/{ => ard}/05_onshore_batch/onshore-batch.ipynbexamples/{ => ard}/05_onshore_batch/run_wind_ard.pyexamples/{ => ard}/06_onshore_multiobjective/inputs/ard_system.yamlexamples/{ => ard}/06_onshore_multiobjective/inputs/windio.yamlexamples/{ => ard}/06_onshore_multiobjective/optimization_demo.ipynbexamples/{ => ard}/data/offshore/GulfOfMaine_bathymetry_100x99.txtexamples/{ => ard}/data/windIO-plant_turbine_IEA-22MW-284m-RWT.yamlexamples/{ => ard}/data/windIO-plant_turbine_IEA-3.4MW-130m-RWT.yamlexamples/{ => ard}/data/windIO-plant_wind-resource_wrg-example.yamlassets/logomaker/inputs/windio.yamltest/ard/system/api/inputs_offshore_floating/ard_system.yamltest/ard/system/api/inputs_offshore_monopile/ard_system.yamltest/ard/system/api/inputs_onshore/ard_system.yamltest/ard/system/collection/test_optiwindnet.pytest/ard/system/cost/test_spacing_approximations_connections.pytest/ard/system/geometry/test_constraints.pytest/ard/system/offshore/test_mooring_packing.pytest/ard/unit/api/inputs_onshore/ard_system_bad_windio.yamltest/ard/unit/api/inputs_onshore/windio.yamltest/ard/unit/cost/test_orbit_wrap.pytest/ard/unit/cost/test_wisdem_wrap.pytest/ard/unit/farm_aero/test_floris.pytest/ard/unit/geographic/test_geomorphology.pytest/flowers/system/rotation/__init__.pytest/flowers/system/rotation/precooked.yamltest/flowers/system/rotation/rotational_consistency.npztest/flowers/system/rotation/sandbox.pytest/flowers/system/rotation/test_rotational_consistency.pytest/flowers/system/rotation/test_rotational_workbench.pytest/flowers/system/test_flowers_stack.pytest/flowers/system/wind_roses/wr4.ptest/flowers/unit/layouts/garbage5.ptest/flowers/unit/test_flowers_model.pytest/flowers/unit/test_tool_discrete_pyrite.npztest/flowers/unit/test_tool_random_pyrite.npztest/flowers/unit/test_tools.pytest/flowers/unit/wind_roses/wr4.p.gitignore: auto-removal of trailing whitespaceard/__init__.py: extraneous addition of experimental "house style" code for matplotlib...