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

Adds classes to structure wind energy data for FLORIS #775

Merged
merged 50 commits into from
Jan 25, 2024

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Jan 12, 2024

@rafmudaf , @misi9170 , @ejsimley, editing this now that it is ready for review

Wind Rose Refactor

Refactor wind rose class to work with new v4 framework. Main points of inclusion in this pull request:

  • Split wind_rose class into two classes, WindRose and TimeSeries, within one module to avoid circular imports
  • Defines a super class WindData which both TimeSeries and WindRose inherit from
  • Provide _unpack() functions that provide the information within either class to floris' reintialize function
  • Provides testing for the new classes and functions in wind_rose_time_series.test.py
  • Ruff formant floris_interface and add capability to pass WindData objects to reinitialize and get_aep functions
  • Adds an example 34 showing new syntax of WindRose and TimeSeries

Context

Originally this pull request was described as addressing more capabilities for the new WindRose TimeSeries but we decided to limit this to just the the above points and upcoming issues/pull requests will address turbulence_intensity based on wind rose

@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 13, 2024

Thanks for the chat today @misi9170 and @rafmudaf , just logging this for when we're back next week. I tried to implement the points we discussed and I added some capability to pass the objects directly to reinitialize (I think it works? There's an example now testing it). One thing though, can reinitialize accept an array of turbulence intensity? Otherwise have a good weekend!

@misi9170
Copy link
Collaborator

One thing though, can reinitialize accept an array of turbulence intensity? Otherwise have a good weekend!

At this stage, the turbulence_intensity keyword argument to reinitialize() is intended to be scalar, I believe. However, this line broadcasts that scalar value to have size n_findex x n_turbines x 1 x 1; so, it is seems that it could be straightforward to provide a different turbulence intensity for each findex, we'd just have to make sure to pass everything through correctly.

@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 16, 2024

One thing though, can reinitialize accept an array of turbulence intensity? Otherwise have a good weekend!

At this stage, the turbulence_intensity keyword argument to reinitialize() is intended to be scalar, I believe. However, this line broadcasts that scalar value to have size n_findex x n_turbines x 1 x 1; so, it is seems that it could be straightforward to provide a different turbulence intensity for each findex, we'd just have to make sure to pass everything through correctly.

That sounds good, I think we should move towards a number of scalar inputs being allowed to be specified for each findex, but we can discuss more

@rafmudaf
Copy link
Collaborator

Thanks @misi9170 and @paulf81, my comments are mostly addressed. Let me know when the remaining comments are addresses, and I'll resolve the review and approve.

@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 18, 2024

Thanks @misi9170 and @paulf81, my comments are mostly addressed. Let me know when the remaining comments are addresses, and I'll resolve the review and approve.

Thank @rafmudaf !

@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 18, 2024

@rafmudaf I think all comments now addressed, thank you!

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.

See the comments I left, but this looks good overall!

@@ -838,6 +851,76 @@ def get_farm_AEP(

return aep

def get_farm_AEP_with_wind_data(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to have a separate function for wind data? Or can wind_data just be an optional argument to get_farm_AEP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is a light wrapper around the other (it calls get_farm_AEP), @misi9170 and I thought this was pretty clean, the other way works too but then you have ambiguous situations depending which values are passed in

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I think that makes sense. But I think this function would be a little easier to use if it didn't require wind_data to "match the wind_data object passed to reinitialize()" as mentioned in the docstring. What if you added the line self.reinitialize(wind_data) so users wouldn't have to call that separately beforehand if they wanted to update the wind conditions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I definitely could! But I think that would then mis-match the behavior of get_AEP, which doesn't accept new wind speeds/directions, it just requires that freq have the same dimensions. The analogy here is that it doesn't use ws/wd but does again require freq to match. I think we'd have to change both functions if we make this change or they will be subtly different if that makes sense? Or I'm overthinking it...

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, no worries! Since that's the convention in get_farm_AEP, no need to change anything.

# turbulence intensity using the unpack_for_reinitialize
# method
if wind_data is not None:
wind_directions, wind_speeds, turbulence_intensity = wind_data.unpack_for_reinitialize()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since floris_interface is expecting a scalar turbulence intensity input, how will it handle an array provided by wind_data.unpack_for_reinitiaize()?

from floris.type_dec import NDArrayFloat


class WindDataBase:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor point: should this inherit from LoggingManager? Or is there no plan to do any logging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually don't know! @rafmudaf or @misi9170, should I it inherit from LoggingManager?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It wouldn't hurt to have the logging functionality available. One immediate use case might be to give some additional information when the length of the WindRose or TimeSeries don't match the length of findex.

wind_speeds: NumPy array of wind speeds (NDArrayFloat).
freq_table: Frequency table for binned wind direction, wind speed
values (NDArrayFloat, optional). Must have dimension
(n_wind_directions, n_wind_speeds). Defaults to None.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest describing what happens by default when freq_table, ti_table, and price_table are None. For example, uniform frequencies and prices are assumed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a good suggestion, implementing now

ti_table: Turbulence intensity table for binned wind direction, wind
speed values (NDArrayFloat, optional). Must have dimension
(n_wind_directions, n_wind_speeds). Defaults to None.
price_table: Price table for binned binned wind direction, wind
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and elsewhere in the module, what do you think about calling this something more generic than "price"? Maybe "value" or "value_of_power"? This could convey that the table could potentially be used to weight the power in each bin to account for a variety of things like environmental or social value, etc.

Also, it would be good to mention how this input is used, for example, that it can be used to weight the power in each bin to compute the total value of the energy produced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! Implementing now,

floris/tools/wind_data.py Show resolved Hide resolved
)

# Only keep the range with values in it
wd_edges = wd_edges[wd_edges + wd_step >= wind_directions_wrapped.min()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on how pd.cut is called with right=False, I think the ">=" should be ">" here (but ">=" is correct in the next line). Is that right? I just considered the case where wd_step = 4 and the minimum wind direction is 2. Then I don't think we need the "-2" bin edge. Same comment for the wind speed case below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, that is a good catch, thank you!

@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 20, 2024

Thank you @ejsimley and @rafmudaf for the round of comments! I think all (except for the one question for you @rafmudaf and @misi9170 on whether WindData should inherit LoggingManager), this one should be good to go? I'll hit request re-review now...

@paulf81 paulf81 requested review from ejsimley and rafmudaf January 20, 2024 05:15
from .wind_rose import WindRose
from .wind_data import (
TimeSeries,
WindDataBase,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think users will be using this base-class, they'll probably only use the derived classes. So we can remove WindDataBase from the user-facing imports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@paulf81 paulf81 requested a review from rafmudaf January 24, 2024 02:14
@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 24, 2024

I fixed the issue that cropped up after not importing Base, let me know if ready to merge, thank you @rafmudaf !

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.

Thanks for updating Paul. I just have one comment on get_farm_AEP_with_wind_data to consider.

@@ -838,6 +851,76 @@ def get_farm_AEP(

return aep

def get_farm_AEP_with_wind_data(
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I think that makes sense. But I think this function would be a little easier to use if it didn't require wind_data to "match the wind_data object passed to reinitialize()" as mentioned in the docstring. What if you added the line self.reinitialize(wind_data) so users wouldn't have to call that separately beforehand if they wanted to update the wind conditions?

@paulf81
Copy link
Collaborator Author

paulf81 commented Jan 24, 2024

thanks @ejsimley ! @rafmudaf, merge city and I'm the mayor?

@rafmudaf rafmudaf changed the title Wind rose refactor Adds classes to structure wind energy data for FLORIS Jan 25, 2024
@rafmudaf rafmudaf merged commit 420fb0f into NREL:v4 Jan 25, 2024
8 checks passed
@paulf81 paulf81 deleted the wind_rose_refactor branch January 25, 2024 18:04
@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
floris.tools in-progress Work is actively in progress v4 Focus of FLORIS v4
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants