search

NREL / flasc

A rich floris-driven suite for SCADA analysis
https://nrel.github.io/flasc/
BSD 3-Clause "New" or "Revised" License
31 stars 18 forks source link

Feature/provide frequency #123

Closed paulf81 closed 11 months ago

paulf81 commented 1 year ago

Could be ready to merge

Feature or improvement description This pull request enables the bin weighting in the energy ratio calculation to come from an externally provided pandas dataframe of weights, rather than necessarily the count of points in the binned data. The assumed implementation provides a dataframe with ws/wd columns matched to the labels inferred from ws_min, ws_step, wd_min etc.,

I had originally assumed that if this table was passed in then the ws/wd bins/edges could be inferred from this table. But in the process of writing the tests realized that an edge cases of single ws or wd might make a challenge here (although solvable) so went back to basically this table just has to match what is otherwise provided by the ws_min etc., parameters. A potential gotcha is for example the user specifies ws_min =0, ws_step = 1, then the bins will be 0.5, 1.5 etc., maybe we can provide helpful warnings?

Otherwise seems to work as is but wanted to maybe use the pull request to discuss how we best handle some of these finer points.

paulf81 commented 1 year ago

Maybe an idea @misi9170 you had was a pre-processor to generate the table, we can pass in an FI wind rose or a table, plus the ws_min/max/step and project so that it will line up with what energy_ratio expects?

Bartdoekemeijer commented 12 months ago

Something I had before was a 2D interpolant function that would take the wind direction and wind speed, and would return the frequency of occurrence. This way, you could calculate the energy ratios using a 20-year-equivalent wind rose even with a smaller amount of data. I thought the interpolant function was particularly helpful because it's agnostic to the binning options chosen. That code snippet is here: https://github.com/NREL/flasc/blob/88330a767d1a91dff49fcef39ea7d1ce67ae526d/flasc/energy_ratio/energy_ratio.py#L45.

Is this more or less what you want to reproduce now with Polars? Sorry for being late to the conversation.

paulf81 commented 12 months ago

Something I had before was a 2D interpolant function that would take the wind direction and wind speed, and would return the frequency of occurrence. This way, you could calculate the energy ratios using a 20-year-equivalent wind rose even with a smaller amount of data. I thought the interpolant function was particularly helpful because it's agnostic to the binning options chosen. That code snippet is here:

https://github.com/NREL/flasc/blob/88330a767d1a91dff49fcef39ea7d1ce67ae526d/flasc/energy_ratio/energy_ratio.py#L45

. Is this more or less what you want to reproduce now with Polars? Sorry for being late to the conversation.

This sounds really nice Bart! Right now I'm just assuming you are passing in a dataframe with the same ws/wd bins identified as in the data. But I like your suggestion, what if for simplicity though we handled this option as a new feature in a separate pull request? I can see expanding on this capability as we go,

Bartdoekemeijer commented 12 months ago

What if for simplicity though we handled this option as a new feature in a separate pull request? I can see expanding on this capability as we go,

Sounds good!

paulf81 commented 12 months ago

What if for simplicity though we handled this option as a new feature in a separate pull request? I can see expanding on this capability as we go,

Sounds good!

Ok opened Issue #125 to capture this idea / plan

paulf81 commented 12 months ago

Ok @ejsimley / @misi9170 @Bartdoekemeijer I think this is now ready to merge from my perspective if 1-2 of you could provide a review. Thank you!

misi9170 commented 11 months ago

Maybe an idea @misi9170 you had was a pre-processor to generate the table, we can pass in an FI wind rose or a table, plus the ws_min/max/step and project so that it will line up with what energy_ratio expects?

Yeah, agreed, just documenting our discussion from today---let's do that, but create a separate PR for it.

paulf81 commented 11 months ago

Maybe an idea @misi9170 you had was a pre-processor to generate the table, we can pass in an FI wind rose or a table, plus the ws_min/max/step and project so that it will line up with what energy_ratio expects?

Yeah, agreed, just documenting our discussion from today---let's do that, but create a separate PR for it.

Done! Created Issue #126

misi9170 commented 11 months ago

@paulf81 Working on creating an extra part of the energy_ratio examples to specify df_freq. Hoping to have that done today, and should be able to provide a review at that time.

misi9170 commented 11 months ago

@paulf81 Have now revamped how df_freq is computed if it is not passed in, passed that through to the Output object, added option to overlay the frequency on the wind direction count plot and shown how to use it in Smarteole examples.

Overall looking good to me, going to approve.

paulf81 commented 11 months ago

thank you @misi9170, these changes look really great and clean things up a lot! @ejsimley back to you now for review when you are available