Skip to content

Improve error handling and robustness in turbine model loads#646

Merged
rafmudaf merged 15 commits intoNatLabRockies:developfrom
rafmudaf:external_input_file
May 5, 2023
Merged

Improve error handling and robustness in turbine model loads#646
rafmudaf merged 15 commits intoNatLabRockies:developfrom
rafmudaf:external_input_file

Conversation

@rafmudaf
Copy link
Copy Markdown
Collaborator

@rafmudaf rafmudaf commented May 5, 2023

Redesign the turbine model loading routine to handle errors and be more robust

An issue was noted were the !include turbine.yaml method of including a turbine model was failing. This was due to how the Farm class was supporting the various methods of loading a turbine model. Turbine definitions could be loaded in three ways:

  1. Pass a string that matches a turbine included in Floris's turbine library
  2. Pass a dictionary representation of a turbine definition
  • Note that there's an option to use the yaml keyword "!include" which results in the yaml library preprocessing the inputs and loading the specified file directly into main input file. The result is that floris sees the turbine definition as a dict.
  1. A string selecting an turbine that exists in an external turbine library specified in turbine_library_path

Currently, if the second option is used, the Farm.turbine_type is a list of dictionaries. However, dictionaries cannot be hashed (i.e. compared) so the subsequent np.unique command in Farm's turbine loading fails. While it may have been possible to find a workaround, I decided to split the work to load the turbine definitions into distinct steps. This pull request does the following to Farm:

  • Check turbine_types with an attrs validator to raise an error if it is not a valid length
  • Enforce type checking with an attrs field-validator for turbine_type and turbine_definitions
  • Creates a turbine definition cache to avoid doing file I/o repeatedly if a definition has already been loaded
  • Treats turbine_types as immutable so that exporting Farm preserves the input data as it was given
  • Adds a unit test for the turbine definition handling

Bonuses:

  • Remove all mention of JSON except from the legacy input reader
  • Expand unit tests for FromDictMixin
  • Let load_yaml function fail if the data given is invalid
  • Consolidate floating and land-based test turbine definitions

Related issue

I don't see an open issue for this.

Impacted areas of the software

Primarily farm.py. Other changes are cosmetic, related to removing JSON, or testing

Test results, if applicable

All tests pass + new tests are added

rafmudaf added 9 commits May 4, 2023 18:06
This should always retain its data as input so that it can be propertly exported
Supporting passing a dict to a function that expects a filepath makes the calling code difficult to read. We should handle the types correctly instead.
This prevents errors when building up a dictionary to create Farm from code
@rafmudaf rafmudaf added bug Something isn't working enhancement An improvement of an existing feature labels May 5, 2023
@rafmudaf rafmudaf added this to the v3.4 milestone May 5, 2023
@rafmudaf rafmudaf requested review from RHammond2, misi9170 and paulf81 May 5, 2023 05:36
@rafmudaf rafmudaf self-assigned this May 5, 2023
@rafmudaf rafmudaf force-pushed the external_input_file branch 2 times, most recently from 7963ca9 to 0be8fe6 Compare May 5, 2023 06:19
rafmudaf added 3 commits May 5, 2023 01:20
@rafmudaf rafmudaf force-pushed the external_input_file branch from 0be8fe6 to 1c8dde0 Compare May 5, 2023 06:20
Copy link
Copy Markdown
Collaborator

@paulf81 paulf81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so impressive @rafmudaf , thank you!! I pulled this version down and confirm it resolves the issue which I was seeing

Copy link
Copy Markdown
Collaborator

@misi9170 misi9170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafmudaf I've put in one suggestion to think about, but it isn't critical to functionality. Everything runs as expected now.

Comment thread floris/simulation/farm.py
Comment thread floris/simulation/floris.py Outdated
Copy link
Copy Markdown
Collaborator

@RHammond2 RHammond2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, especially the additional commentary on what's going on in each step. I was originally unsure of the change in control flow, but agree that with the expanded commentary it's worthwhile.

I would just add that one keyword argument, and then this is good to go from my perspective.

@rafmudaf
Copy link
Copy Markdown
Collaborator Author

rafmudaf commented May 5, 2023

Thanks for the reviews @RHammond2 @misi9170 @paulf81

@rafmudaf rafmudaf merged commit ab03282 into NatLabRockies:develop May 5, 2023
@rafmudaf rafmudaf deleted the external_input_file branch May 5, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement An improvement of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants