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] separate panel uncertainty plot #779

Conversation

LeonieFreisinger
Copy link
Collaborator

Referencing issue #710
Problem
The plot() function already includes plotting the uncertainty. However, the plot_components() function does not include plotting the uncertainty of yhat as a component.

Solution
Modify the plot_components() function to plot the uncertainty of yhat as a component in a separate panel. Therefore, the matplotlib and plotly version will be modified. The uncertainty will be plotted with the x-axis = 0.5 quantile as reference.

@LeonieFreisinger LeonieFreisinger marked this pull request as draft September 20, 2022 15:00
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

Merging #779 (80dc5cf) into main (aa99674) will increase coverage by 0.05%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##             main     #779      +/-   ##
==========================================
+ Coverage   87.00%   87.06%   +0.05%     
==========================================
  Files          16       16              
  Lines        4580     4622      +42     
==========================================
+ Hits         3985     4024      +39     
- Misses        595      598       +3     
Impacted Files Coverage Δ
neuralprophet/forecaster.py 86.26% <62.50%> (-0.22%) ⬇️
neuralprophet/plot_forecast.py 75.11% <95.83%> (+2.20%) ⬆️
neuralprophet/plot_forecast_plotly.py 87.55% <100.00%> (+1.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Kevin-Chen0 Kevin-Chen0 linked an issue Sep 20, 2022 that may be closed by this pull request
@Kevin-Chen0 Kevin-Chen0 added this to the Sprint 0.3.3b milestone Sep 20, 2022
@Kevin-Chen0
Copy link
Collaborator

Thanks for all your work @LeonieFreisinger! I'll review this shortly and will give you feedback if any.

@LeonieFreisinger
Copy link
Collaborator Author

@Kevin-Chen0 Thanks a lot!! :) I just added an additional test in test_plotting.py to improve code cov.

@Kevin-Chen0 Kevin-Chen0 marked this pull request as ready for review September 21, 2022 10:09
@ourownstory
Copy link
Owner

ourownstory commented Sep 21, 2022

Some improvement ideas:

  • Use same color schema as in main uncertainty plotting (as in plotly)
  • require highlight_nth_step_ahead when n_forecasts > 1 and quantiles in use

@Kevin-Chen0
Copy link
Collaborator

Kevin-Chen0 commented Sep 22, 2022

Hey Leonie, maybe you can also modify the uncertainty_estimation_peyton_manning.ipynb example notebook so the m.plot_components(forecast) are so plotting the uncertainties. This notebook is a good way to illustrate your changes around uncertainty/quantiles.

@Kevin-Chen0
Copy link
Collaborator

Also, should the m.plot_parameters() include uncertainty as well or just m.plot_components(forecast)?

@LeonieFreisinger
Copy link
Collaborator Author

@Kevin-Chen0 @ourownstory thanks a lot!
I will modify the notebook. I think the uncertainties shouldn´t be in the m.plot_parameters()

@LeonieFreisinger LeonieFreisinger changed the title 710 feature/separate panel uncertainty plot [feature] separate panel uncertainty plot Sep 28, 2022
@LeonieFreisinger
Copy link
Collaborator Author

@ourownstory code check fails because of test_regularization_lagged_regressor. This should be fixed when merging #794 into main and rebasing this branch.

@ourownstory
Copy link
Owner

Also, should the m.plot_parameters() include uncertainty as well or just m.plot_components(forecast)?

we could allow the user to decide to plot a specific percentile, instead of hard-coding the mean.

Copy link
Owner

@ourownstory ourownstory left a comment

Choose a reason for hiding this comment

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

In all your string formatings, please use f-strings.

neuralprophet/plot_forecast.py Outdated Show resolved Hide resolved
@ourownstory
Copy link
Owner

Please also remove warnings printouts from the tutorial notebooks.

@LeonieFreisinger
Copy link
Collaborator Author

LeonieFreisinger commented Oct 17, 2022

Also, should the m.plot_parameters() include uncertainty as well or just m.plot_components(forecast)?

we could allow the user to decide to plot a specific percentile, instead of hard-coding the mean.

okay. I would suggest following up on that feature in a new PR. Is that okay? @ourownstory

@LeonieFreisinger LeonieFreisinger requested review from ourownstory and removed request for Kevin-Chen0 October 17, 2022 07:59
Copy link
Owner

@ourownstory ourownstory left a comment

Choose a reason for hiding this comment

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

LGTM

@ourownstory ourownstory merged commit f7e09e4 into ourownstory:main Oct 18, 2022
@LeonieFreisinger LeonieFreisinger deleted the 710-feature/separate_panel_uncertainty_plot branch November 30, 2022 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Feature: add uncertainty as a panel to plot_components
4 participants