[enhancement] add pytests for matplotlib

[LeonieFreisinger]

Problem
pytests in test_plotting() only cover plotly and do not cover matplotlib plotting backend.

Solution
Adapt all existing pytests to cover the plotly and matplotlib plotting backend.

• I have performed a self-review of my own code.
• I have commented my code, added Docstrings and data types to function definitions.
• My code follows the style guidelines described under CONTRIBUTING.md.
• I have added pytests to check whether my feature / fix works
• All gh actions pass or the failure is not related to my code.

[codecov-commenter]

Codecov Report

Merging #914 (0f801c4) into main (bba5a67) will increase coverage by 1.30%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #914      +/-   ##
==========================================
+ Coverage   87.91%   89.21%   +1.30%     
==========================================
  Files          17       17              
  Lines        4443     4443              
==========================================
+ Hits         3906     3964      +58     
+ Misses        537      479      -58     

Impacted Files	Coverage Δ
neuralprophet/forecaster.py	87.77% <0.00%> (-0.35%)	⬇️
neuralprophet/plot_forecast_matplotlib.py	81.66% <0.00%> (+3.33%)	⬆️
neuralprophet/plot_model_parameters_matplotlib.py	86.89% <0.00%> (+18.96%)	⬆️

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

[LeonieFreisinger]

@karl-richter I transferred all plotting-related test functions to the test_plotting and parametrized both backends. Would be great if you can review the changes since you are well familiar with the plotting functionalities. Thank you!

@Kevin-Chen0 I marked this PR for the upcoming sprint.

[karl-richter]

Looks really good to me!
The only thing we should consider performance wise - does it really make sense to run the same test twice? since technically all parameters are the same and only the output is visualized using two frameworks we could also loop through the plottling backends within the tests. This should technically be faster. However, the current method provides more visibility. Not sure if you have discussed this with oskar yet.

[LeonieFreisinger]

@karl-richter thanks a lot for your feedback, definitely a valid point regarding the performance. I discussed with @ourownstory about this. Since the code between the plotly and matplotlib functions differs at some places, it would be great to have it checked/ covered. If we decide to drop one of the backends or see the need for increasing the performance by reducing the tests, we can do so by simply dropping the matplotlib parameter in the tests.

[Kevin-Chen0]

Hi Leonie, can you take a look and see why the model / metrics (pull_request) is failing? You can sync up with @judussoari and @karl-richter about this.

[LeonieFreisinger]

Hi Leonie, can you take a look and see why the model / metrics (pull_request) is failing? You can sync up with @judussoari and @karl-richter about this.

@Kevin-Chen0 the metric test currently fails in all my pull request. However, there is already a fix on the cml repo iterative/cml#1228. As soon as this PR will be deployed, the error will resolve.

[Kevin-Chen0]

I am wondering whether you can use matplotlib.isinteractive() from PR #960 instead of having a PLOT variable?

[LeonieFreisinger]

@Kevin-Chen0 Thanks for bringing up the idea. Yes, I can add the matplotlib.isinteractive(). Should we fully remove the PLOT variable? Then we would not have a manual switch to turn the plotting out/on anymore. Would it be an idea to add the matplotlib.isinteractive() to the if queries?

[LeonieFreisinger]

@karl-richter would be great to quickly have your opinion on it.

[Kevin-Chen0]

If we have matplotlib.isinteractive(), then we should remove the PLOT variables in the tests, as it is hardcoded as False by default for the test. @karl-richter thoughts?

[LeonieFreisinger]

@karl-richter would be amazing if you could merge this PR. Please, wait for the PR#844 to be merged (by @noxan). In this order, we will have an easier time with merge conflicts. Thank you!