Skip to content
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

Raise informative errors if v3 input files passed in #829

Merged
merged 8 commits into from
Mar 4, 2024

Conversation

misi9170
Copy link
Collaborator

@misi9170 misi9170 commented Mar 3, 2024

Addresses #791

Checklist:

Notes and discussion points

  • Can the converter utilities (for both turbine and the main floris input files) be used if Floris is installed from PyPI? If not, do we need to somehow add support for that?
  • The check for a v3 floris input only raises an informative error if Floris is instantiated from a file---a more generic error will be raised if Floris is instantiated from a dictionary. I think this is OK, but I wanted to flag it here.
  • We don't currently have a converter for cp -> power conversion for multidimensional turbines (where the cp/power curves are specified on a separate csv file). I think this is a small enough use case for the time being that we can ask users to do their own conversion, but again, I wanted to flag it.

@misi9170 misi9170 marked this pull request as draft March 3, 2024 15:13
@misi9170 misi9170 marked this pull request as ready for review March 3, 2024 17:35
@misi9170 misi9170 added enhancement An improvement of an existing feature v4 Focus of FLORIS v4 labels Mar 3, 2024
@misi9170 misi9170 added this to the v4.0 milestone Mar 3, 2024
@misi9170 misi9170 self-assigned this Mar 3, 2024
Copy link
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 really great and very helpful, thank you @misi9170 !

@paulf81
Copy link
Collaborator

paulf81 commented Mar 4, 2024

hi @misi9170 , I went through the code and thought it all looks great, thank you! I approved. I agree with your 2nd and 3rd comments that this is an ok way to leave things. Then on pip install, there I don't know, maybe something to see in beta testing?

@rafmudaf
Copy link
Collaborator

rafmudaf commented Mar 4, 2024

I like the spirit of this pull request, and I think it's a very helpful addition. Architecturally, this is mixing the physics model with the software infrastructure. I suggest to keep anything related to versioning, especially things you'll want to go back and change later as new versions come up, all in one place at the outer-most layer of the architecture. There was a place for this in the Floris class from the v2 to v3 shift (https://github.com/NREL/floris/blob/develop/floris/simulation/floris.py#L167). Essentially, when we read and deal with the Farm class in the future, we're not going to care about the input checking and messaging, so this adds more noise for our brains. Keeping it at the top allows us to forget about it until we need to go find it.

That being said, I'm happy with your decision on how to handle it.

@misi9170
Copy link
Collaborator Author

misi9170 commented Mar 4, 2024

To help address my third point above, I've now added an explanation that the user will need to do their own conversion of the multidimensional turbine csv file if they call convert_floris_input_v3_to_v4.py on a floris input file that specifies multidim_cp_ct as the velocity model, as well as raising an error and an explanation if the user runs convert_turbine_v3_to_v4.py on a multidimensional turbine yaml.

@rafmudaf
Copy link
Collaborator

rafmudaf commented Mar 4, 2024

Regarding #829 (comment), @misi9170 and I looked at the options for catching outdated turbine inputs at a higher level in the architecture of FLORIS. The ideal time is prior to initializing the Floris object so in Floris.from_file or Floris.from_dict.

turbine_type can be either a list of dictionaries or a list of strings for the turbine names. If it's a list of dictionaries, inspecting the data is straightforward. However, if it's a list of strings referring to turbine names, then the turbine data are not yet available for inspection. This is specifically a problem when using the external turbine library feature.

Rather than trying to hack something together that is more complicated, we agreed it's best to have a more robust check within the Farm class rather than at the Floris class level.

@misi9170 misi9170 merged commit 57ec83c into NREL:v4 Mar 4, 2024
8 checks passed
@misi9170 misi9170 deleted the v4-ms/raise-v3-error branch March 5, 2024 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature v4 Focus of FLORIS v4
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants