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/add uplift example #155

Merged
merged 7 commits into from
Dec 19, 2023
Merged

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Dec 18, 2023

Feature or improvement description
This pull request adds an additional example demonstrating the calculation of total uplift. I think it's mostly ready but there are a few finer points I'd like to work through before merging:

  • What are the warnings about? Can they be squashed
  • Should we lock in a random seed to guarantee one result so the documentation can be more specific?

Related issue, if one exists
Closes Issue #138

@misi9170
Copy link
Collaborator

misi9170 commented Dec 19, 2023

Nice! On your points above:

  • I'm strongly in favor of adding np.random.seed(0) right after if __name__ == "__main__": for reproducibility
  • Did you mean to leave plt.show() off? There are a lot of plots made in this script that are abandoned
  • I'm still looking into the warnings, but they all originate from the plot_binned_mean_and_ci call, so if you don't want to plot anything anyway that line can be commented out and the warnings don't appear. On the other hand, it's probably best to track those down and see what causes them?

EDIT: I've now fixed the warnings, I think. In doing so, I made the following changes:

  • Renamed examples 05 and 06, because there was a double-up at 05 (likely my fault when reorganizing the examples)
  • Added random seeds to both 06 and 07 (new numbering)
  • Specified the x_edges passed to plot_binned_mean_and_ci(), which avoids empty bins caused by default edges
  • Added handling for bins with no spread to plot_binned_mean_and_ci(), which is possibly not needed for realistic data but avoids warnings raised with the synthetic data that is too uniform

This fixes all warnings that I was seeing.

@paulf81
Copy link
Collaborator Author

paulf81 commented Dec 19, 2023

Thanks @misi9170 ! I know you're making some last checks but feel free to merge when done, thank you!

@misi9170 misi9170 merged commit 3ab0fb2 into NREL:develop Dec 19, 2023
3 checks passed
@paulf81 paulf81 deleted the feature/add_uplift_example branch December 19, 2023 22:52
@misi9170 misi9170 mentioned this pull request Dec 21, 2023
@misi9170 misi9170 mentioned this pull request Apr 10, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants