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

Convert turbulence intensity from single value to n_findex length array #782

Merged
merged 106 commits into from Feb 3, 2024

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Jan 19, 2024

Convert turbulence intensity from single value to n_findex length array

This pull request builds on Pull Request #775 , and is intended to address Issue #778 and by extension issues #666 and #713. It converts turbulence_intensity (float) to turbulence_intensities (np.array). Further it enables turbulence_intensities to be specified:

  • As an array within the yaml
  • As an array to fi.reinitialize
  • As an array to the TimeSeries object
  • As a wd x ws table to WindRose object

Or to be generated via helper functions within the WindRose and TimeSeries objects. In more detail:

  • Rename flow_field's turbulence_intensity to turbulence_intensities
  • Refactor the simulation files to expect turbulence_intensities to be an array, rather than a float
  • Add additional tests of turbulence intensity to confirm correct behavior of new features
  • Complete WindRose and TimeSeries handling of turbulence intensities
  • Add helper functions to WindRose and TimeSeries which allow turbulence intensities to be generated, rather than provided, as a function of wind directions and wind speeds
  • Add additional examples of usage

All tests passing.

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 2, 2024

ok @ejsimley @rafmudaf and @misi9170 , I think all comments addressed, good to merge and clean shop before the weekend? Thanks!!

@misi9170
Copy link
Collaborator

misi9170 commented Feb 2, 2024

@paulf81 I was just cleaning up the plotting a bit for example 35, and noticed that the rear turbine power doesn't vary very smoothly with TI:

image

Is that expected? If not, it's likely not an issue with this PR per se, but something we may need to look into

Copy link
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.

I think we should revert some of the legacy files to turbulence_intensity to avoid confusion about forward compatibility; also, I'm not sure whether we need a v3 to v4 input file converter now or whether we should raise a less generic error if users still have turbulence_intensity on their input files

@@ -112,7 +112,8 @@ flow_field:

###
# The level of turbulence intensity level in the wind.
turbulence_intensity: 0.06
turbulence_intensities:
- 0.06
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only change so far to the floris input files from v3 to v4 (that turbulence_intensity, a scalar, changes to turbulence_intensities, a list). When running the new code with an v3 input file, the generic error gets raised:

AttributeError: The initialization for FlowField was given extraneous inputs: ['turbulence_intensity']

Is that OK, or do we need either a converter to convert v3 models to v4 (similar to what we now have for v3-style turbine models) or a more explicit explanation of the change in the error raised?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both sound good to me, we could add a specific error for when turbulence_intensity is provided, and maybe (with a seperate issue) prepare a converter?

floris/tools/floris_interface_legacy_reader.py Outdated Show resolved Hide resolved
floris/tools/optimization/legacy/scipy/yaw_wind_rose.py Outdated Show resolved Hide resolved
@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 2, 2024

@paulf81 I was just cleaning up the plotting a bit for example 35, and noticed that the rear turbine power doesn't vary very smoothly with TI:

image

Is that expected? If not, it's likely not an issue with this PR per se, but something we may need to look into

I think this looks like the near-wake boundary in GCH, which is has this sharp change, my guess is that length of the near wake depends on TI so we cross it and transition from linear to gaussian wake

@misi9170
Copy link
Collaborator

misi9170 commented Feb 2, 2024

@paulf81 I was just cleaning up the plotting a bit for example 35, and noticed that the rear turbine power doesn't vary very smoothly with TI:
image
Is that expected? If not, it's likely not an issue with this PR per se, but something we may need to look into

I think this looks like the near-wake boundary in GCH, which is has this sharp change, my guess is that length of the near wake depends on TI so we cross it and transition from linear to gaussian wake

That would make sense. Is there also another more subtle kink at about 14% TI, or am I imagining that?

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 2, 2024

@paulf81 I was just cleaning up the plotting a bit for example 35, and noticed that the rear turbine power doesn't vary very smoothly with TI:
image
Is that expected? If not, it's likely not an issue with this PR per se, but something we may need to look into

I think this looks like the near-wake boundary in GCH, which is has this sharp change, my guess is that length of the near wake depends on TI so we cross it and transition from linear to gaussian wake

That would make sense. Is there also another more subtle kink at about 14% TI, or am I imagining that?

There might be an inflection there, but I don't 'have a ready answer if so...

@paulf81 paulf81 requested a review from misi9170 February 2, 2024 21:16
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just want to point out that we're adding more and more example. This one and 36 are both fairly short and simple, and I could see them easily being consolidated into one, worked into one of 4 through 6, or being included in the Getting Started notebook. In any case, these should be described in the examples listing in the documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh definitely, I don't think the examples should stay as they are now, but reconsolidated in a separate pull request, but I was saving that effort until nearer the end when all the tools are ready. Examples 35 and 36 will be folded into the earlier examples but for now I just wanted to produce some temp example

Copy link
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.

This looks fine to me. I'm OK for this to be merged, with the understanding that there may be some to do items left for later PRs:

  • possibly raising a specific error if turbulence_intensity is specified in the input yaml, and/or creating a converter to write new v4 yamls
  • consolidating/removing the examples, which can happen when we propagate the TimeSeries and WindRose data structures more thoroughly through the examples
  • creating an issue for that min operation in the YawOptimizerBase.

Copy link
Collaborator

@ejsimley ejsimley left a 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. I'd suggest creating issues or at least project board cards for the points @misi9170 listed before merging so we don't lose track of them.

@paulf81 paulf81 requested a review from rafmudaf February 2, 2024 23:23
@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 2, 2024

Thanks guys! I can do those new issues before merging, ok for you @rafmudaf ?

This makes the if-statement match the comment above it. By removing a couple of levels if indentation due to the if-statements, the full condition is more clear
@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 3, 2024

@rafmudaf @ejsimley @misi9170 @bayc

I've added an issue and several project cards to track the issues not fully resolved by this pull request.

Issue #791
https://github.com/orgs/NREL/projects/96/views/2?pane=issue&itemId=52095313
https://github.com/orgs/NREL/projects/96/views/2?pane=issue&itemId=52095412
https://github.com/orgs/NREL/projects/96/views/2?pane=issue&itemId=52095420

Have put these all toward the end of the process because I believe they are best done after other actions are completed. But it's important we not forget so including these here.

@paulf81 paulf81 merged commit 61e1f13 into NREL:v4 Feb 3, 2024
8 checks passed
@paulf81 paulf81 deleted the wind_rose_ti_vary branch February 3, 2024 05:05
@misi9170 misi9170 mentioned this pull request Apr 8, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 Focus of FLORIS v4
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants