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

Improvements to WindRose resampling #857

Merged
merged 24 commits into from
Apr 9, 2024

Conversation

misi9170
Copy link
Collaborator

@misi9170 misi9170 commented Mar 27, 2024

@Bartdoekemeijer noted a few place for improvement in the WindRose class, in particular:

  1. Potentially unexpected behavior of the resample_wind_rose() method, which returns a new instantiation rather than modifying the current object
  2. Unexpected default behavior of plot_wind_rose(), which resamples the data provided by default so makes it ambiguous which bins were specified (also, it turns out that providing wd_step=None to avoid resampling causes plot_wind_rose() to fail).
  3. Minimal support for resampling to a higher bin resolution

This PR addresses those issues.

  • inplace option provided for resample_wind_rose() to modify the current object (defaults to False at the moment to retain former operation, but we could consider changing the default to True, which may be more intuitive. However, defaulting to False is more pandas-like).
  • Default for plot_wind_rose() is now not to resample
  • Rename plot_wind_rose() and resample_wind_rose() to plot() and resample() for consistency across WindRose and WindTIRose() (and for simplicity)
  • Enhanced resampling to a higher bin resolution (@paulf81 , I'm not sure that this is going to work with the current method of creating a TimeSeries and calling to_WindRose---any ideas here?)*
  • Map changes onto WindTIRose @paulf81

* We may need to be careful here, as there are several ways we could interpolate the data, so we should take a conservative/simple approach (but less conservative than the current approach, which is simply to assign extra 0 frequency bins)

@misi9170 misi9170 added enhancement An improvement of an existing feature v4 Focus of FLORIS v4 labels Mar 27, 2024
@misi9170 misi9170 changed the title Feature/enhance windrose resampling Improvements to WindRose resampling Mar 27, 2024
@paulf81
Copy link
Collaborator

paulf81 commented Apr 4, 2024

@misi9170 and @Bartdoekemeijer (and @ejsimley if you time?) could you check quick what you think of the commit I just pushed up to address the point above on enhanced resampling for upsampling? You'll see I made the following changes:

  • I changed the name of resampling to aggregating which I think more accurately captures what it does
  • I set a check that aggregating can only go to larger bins
  • I added a new function resample_by_interpolation, which works like the interpolate function in v3 FLORIS.

I'll need to next follow-through on the code if you like it in principle, or can change things. But next steps will be:

  • Make matching changes to WindTIRose
  • Update existing tests and maybe add a couple tests
  • Update examples and perhaps documentation (although that is in another pull request)

if ws_step is not None:
if len(self.wind_speeds) >= 2:
if ws_step < self.wind_speeds[1] - self.wind_speeds[0]:
raise ValueError("ws_step must be at least as large as the current step")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@paulf81 maybe here, we should print what the current step is, which might help them set it correctly?
something like:

current_ws_step = self.wind_speeds[1] - self.wind_speeds[0]
if ws_step < current_ws_step:
    raise ValueError("ws_step provided must be at least as large as the current ws_step ({0:.2f} m/s)".format(current_ws_step)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, making the change...

elif method == "nearest":
interpolator = NearestNDInterpolator
else:
UserWarning("Unknown interpolation method: '{:s}'".format(method))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UserWarning("Unknown interpolation method: '{:s}'. Available methods are 'linear' and 'nearest'".format(method))

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually what is a "UserWarning"? does it cause an error? If not, what happens next?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oof, this should be a ValueError, switching

@misi9170
Copy link
Collaborator Author

misi9170 commented Apr 5, 2024

@paulf81 I like your suggestion above!

@misi9170 misi9170 added this to the v4.0 milestone Apr 7, 2024
@misi9170 misi9170 self-assigned this Apr 7, 2024
@ejsimley
Copy link
Collaborator

ejsimley commented Apr 9, 2024

@paulf81, I reviewed the WindData aggregate and resample_by_interpolation methods and like the general idea of splitting the resampling into those two functions and like how they're implemented too.

@paulf81
Copy link
Collaborator

paulf81 commented Apr 9, 2024

Ok @misi9170 , assuming all green checks above I think this is all good to go. The one hedge is I did update the TiRose resample -> aggregate (mostly a name change but added checking that we aren't upsampling) but didn't add an interpolate method here, fearing the 3D interpolation and being short on time, can come back later for this?

@misi9170 misi9170 marked this pull request as ready for review April 9, 2024 18:02
@misi9170 misi9170 merged commit b10d091 into NREL:v4 Apr 9, 2024
8 checks passed
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