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

Feature/provide frequency #123

Merged
merged 24 commits into from
Oct 13, 2023
Merged

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Sep 11, 2023

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 paulf81 added the enhancement An improvement of an existing feature label Sep 11, 2023
@paulf81 paulf81 added this to the v1.4 milestone Sep 11, 2023
@paulf81 paulf81 self-assigned this Sep 11, 2023
@paulf81
Copy link
Collaborator Author

paulf81 commented Sep 11, 2023

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?

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.

Overall, looks good Paul. Just a few comments.

I didn't follow this statement, though: "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?" What is the problem with this case?

flasc/energy_ratio/energy_ratio.py Outdated Show resolved Hide resolved
flasc/energy_ratio/energy_ratio.py Show resolved Hide resolved
flasc/energy_ratio/energy_ratio.py Outdated Show resolved Hide resolved
@Bartdoekemeijer
Copy link
Collaborator

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:

inflow_freq_interpolant (interpolant, optional): This is an
.

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

@paulf81
Copy link
Collaborator Author

paulf81 commented Sep 19, 2023

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:

inflow_freq_interpolant (interpolant, optional): This is an

.
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,

@paulf81 paulf81 requested a review from ejsimley September 19, 2023 22:07
@Bartdoekemeijer
Copy link
Collaborator

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
Copy link
Collaborator Author

paulf81 commented Sep 20, 2023

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
Copy link
Collaborator Author

paulf81 commented Sep 20, 2023

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

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
Copy link
Collaborator Author

paulf81 commented Sep 25, 2023

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

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

paulf81 commented Sep 27, 2023

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

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.

This looks good to me! Sorry for the delay in reviewing. Just a few small documentation comments, and some questions/ideas that aren't necessary to address now.

One thing to fix, though, is that there is an error in the "Comparison assuming no wind direction uncertainty/variability in FLORIS" section of the example 06 notebook.

flasc/energy_ratio/energy_ratio.py Outdated Show resolved Hide resolved


# Enforce that each ws/wd bin combination has to appear in all dataframes
.filter(pl.count().over(bin_cols_without_df_name) == num_df)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe not important to address now, but really we just want to make sure each ws/wd bin combination appears in both data frames for each uplift pair right, not all data frames in the case where there are more than two?

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 think this implementation is the most general, say you want to compare a data partioned into 3 stability types, this checks that each type has at least one point in the used bins. I think it's a fair qualifier.

tests/energy_ratio_test.py Outdated Show resolved Hide resolved
flasc/energy_ratio/energy_ratio.py Show resolved Hide resolved
@misi9170 misi9170 merged commit 34dcddb into NREL:develop Oct 13, 2023
@paulf81 paulf81 deleted the feature/provide_frequency branch October 16, 2023 20:16
@misi9170 misi9170 mentioned this pull request Oct 26, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants