Feature/add options on count #122
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
misi9170
approved these changes
Sep 8, 2023
ejsimley
approved these changes
Sep 11, 2023
flasc/energy_ratio/energy_ratio.py
Outdated
| @@ -186,6 +193,10 @@ def _compute_energy_ratio_bootstrap(er_in, | |||
| ws_min (float): The minimum wind speed to use. | |||
| ws_max (float): The maximum wind speed to use. | |||
| bin_cols_in (list[str]): A list of column names to use for the wind speed and wind direction bins. | |||
| weight_by (str): How to weight the energy ratio, options are 'min', or 'sum'. 'min' means | |||
| the minimum count across the dataframes is used to weight the energy ratio. 'mean' means the mean | |||
| count across the dataframes is used to weight the energy ratio. 'sum' means the sum of the counts | |||
There was a problem hiding this comment.
I think this line about 'mean' means the mean count across... should be removed. It doesn't seem like the 'mean' option is implemented anywhere.
| @@ -58,6 +59,9 @@ def __init__(self, | |||
| ws_min (float): The minimum wind speed value. | |||
| ws_max (float): The maximum wind speed value. | |||
| bin_cols_in (List[str]): TBD | |||
| weight_by (str): How to weight the energy ratio, options are 'min', , or 'sum'. 'min' means | |||
|
Seems very useful! |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ready to be merged
Feature or improvement description
This pull request changes the current behavior that assumes, when weighting wind speed bins in the energy ratio across dataframes, that we should use the minimum count (thus up-weighting the wind-speed bins that have the most points in the lesser case), allow an option to weight by sum. Minimum will stay the default but now an option to pass in 'sum' to a new weight_by variable.
The pull request includes a new test which shows and checks the functionality on a small problem
Finally, a small unrelated bug in energy_ratio_output (remove_all_bins used instead of remove_all_nulls is fixed) and the energy_ratio example for wake steering is cleaned up.
Additional supporting information
This is in support of functions to compute total uplift
Test results, if applicable
New test added test_alternative_weighting, all tests pass