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

[refactor] Extract plot utils into dedicated file #1000

Merged
merged 7 commits into from
Nov 30, 2022

Conversation

LeonieFreisinger
Copy link
Collaborator

@LeonieFreisinger LeonieFreisinger commented Nov 29, 2022

Problem referring #965
Helper functions in plotting related files are defined in actual plotting files. This enlarges the file content.

Solution
Relocate plotting-related helper functions to a separate file utils_plot.py

Key Changes:

  • Check the plotting-related .py files for helper functions and move them to utils_plot.py
  • Check utils.py for plotting-related helper functions and move them to utils_plot.py

Functions that were not relocated:
plot_trend_change(), plot_trend(), plot_daily(), plot_weekly(), plot_yearly(), plot_custom_season(), plot_lagged_weights(), plot_scalar_weights(), plot_forecast_component(), plot_multiforecast_component()

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2022

Codecov Report

Merging #1000 (c6e1bed) into main (e592381) will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##             main    #1000   +/-   ##
=======================================
  Coverage   89.88%   89.89%           
=======================================
  Files          18       19    +1     
  Lines        4655     4659    +4     
=======================================
+ Hits         4184     4188    +4     
  Misses        471      471           
Impacted Files Coverage Δ
neuralprophet/utils.py 81.08% <ø> (-0.34%) ⬇️
neuralprophet/plot_utils.py 89.77% <89.77%> (ø)
neuralprophet/forecaster.py 88.01% <100.00%> (ø)
neuralprophet/plot_forecast_matplotlib.py 81.66% <100.00%> (ø)
neuralprophet/plot_forecast_plotly.py 86.82% <100.00%> (-0.77%) ⬇️
neuralprophet/plot_model_parameters_matplotlib.py 90.50% <100.00%> (-0.24%) ⬇️
neuralprophet/plot_model_parameters_plotly.py 94.86% <100.00%> (ø)

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

@LeonieFreisinger
Copy link
Collaborator Author

@noxan would be amazing if you can review this PR. Should be a fast one.
P.S: I did not relocate the functions that call a plot command, since they in my eyes are not a helper function.

Thanks a lot!

@LeonieFreisinger LeonieFreisinger marked this pull request as ready for review November 29, 2022 11:52
@LeonieFreisinger LeonieFreisinger added the status: needs review PR needs to be reviewed by Reviewer(s) label Nov 29, 2022
Copy link
Collaborator

@noxan noxan left a comment

Choose a reason for hiding this comment

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

LGTM - one note on this massive function to build the component plotting configuration (it is too massive) for a follow up and just wondering: would plot_utils.py be a better name? Otherwise good to go from my side.

valid_configuration = {
"components_list": plot_components,
}
return valid_configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

At some point we should definitely refactor this method... also in parts related to #1004

@noxan noxan added status: ready PR is ready to be merged and removed status: needs review PR needs to be reviewed by Reviewer(s) labels Nov 30, 2022
@noxan
Copy link
Collaborator

noxan commented Nov 30, 2022

@LeonieFreisinger let me know what you think and otherwise I'd merge it directly (possibly create follow up issues).

@noxan noxan changed the title Plot utils [refactor] Extract plot utils into dedicated file Nov 30, 2022
@noxan
Copy link
Collaborator

noxan commented Nov 30, 2022

PS: Congrats on securing pull request #1000 - 🎉 💯0

@LeonieFreisinger
Copy link
Collaborator Author

LGTM - one note on this massive function to build the component plotting configuration (it is too massive) for a follow up and just wondering: would plot_utils.py be a better name? Otherwise good to go from my side.

@noxan thanks for the fast review. Initially planned to name the module plot_utils.py. However, I aligned it with utils.py and utils_torch.py. I agree with changing it. Should we rename the utils_torch.py as well to torch_utils.py?
Then I open a new issue for it to keep it separate. As well as for the get_valid_configuration() and link it to #1004.

@noxan
Copy link
Collaborator

noxan commented Nov 30, 2022

Thanks @LeonieFreisinger - good point about having consistent naming for our util* files, let's take the discussion to the issue you created :)

@noxan noxan merged commit af344bf into ourownstory:main Nov 30, 2022
@github-actions
Copy link

af344bf

Model Benchmark

Benchmark Metric main current diff
YosemiteTemps MAE_val 1.72949 1.72948 -0.0%
YosemiteTemps RMSE_val 2.27386 2.27386 -0.0%
YosemiteTemps Loss_val 0.00096 0.00096 -0.0%
YosemiteTemps RegLoss_val 0 0 0.0%
YosemiteTemps epoch 84 84 0.0%
YosemiteTemps MAE 1.45189 1.45189 -0.0%
YosemiteTemps RMSE 2.16631 2.16631 -0.0%
YosemiteTemps Loss 0.00066 0.00066 0.0%
YosemiteTemps RegLoss 0 0 0.0%
YosemiteTemps time 93.4 153.57 64.42% ⚠️
PeytonManning MAE_val 0.64636 0.64636 -0.0%
PeytonManning RMSE_val 0.79276 0.79276 -0.0%
PeytonManning Loss_val 0.01494 0.01494 -0.0%
PeytonManning RegLoss_val 0 0 0.0%
PeytonManning epoch 37 37 0.0%
PeytonManning MAE 0.42701 0.42701 0.0%
PeytonManning RMSE 0.57032 0.57032 -0.0%
PeytonManning Loss 0.00635 0.00635 0.0%
PeytonManning RegLoss 0 0 0.0%
PeytonManning time 11.7 18.97 62.14% ⚠️
AirPassengers MAE_val 15.2698 15.2698 0.0%
AirPassengers RMSE_val 19.4209 19.4209 0.0%
AirPassengers Loss_val 0.00195 0.00195 0.0%
AirPassengers RegLoss_val 0 0 0.0%
AirPassengers epoch 89 89 0.0%
AirPassengers MAE 9.82902 9.82902 0.0%
AirPassengers RMSE 11.7005 11.7005 0.0%
AirPassengers Loss 0.00056 0.00056 0.0%
AirPassengers RegLoss 0 0 0.0%
AirPassengers time 4.51 7.18 59.2% ⚠️
Model training plots

Model Training

PeytonManning

YosemiteTemps

AirPassengers

@ourownstory
Copy link
Owner

Congratulations @LeonieFreisinger you merged our #1000 th Pull Request! You won a free tour around Stanford campus! Please let me know your availability.

@LeonieFreisinger
Copy link
Collaborator Author

Congratulations @LeonieFreisinger you merged our #1000 th Pull Request! You won a free tour around Stanford campus! Please let me know your availability.

definitely feel honored of saving PR1000. hahhaha @ourownstory almost as good as winning the lottery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move plotting-related utility functions to plot_utils.py
4 participants