68 path optional plot forecast #71
Conversation
anticipy/forecast_plot.py
Outdated
| def plot_forecast(df_fcast, path, output='html', width=None, | ||
| height=None, title=None, dpi=70, show_legend=True, | ||
| auto_open=False): | ||
| def plot_forecast(df_fcast, output, path, width=None, height=None, |
There was a problem hiding this comment.
Shouldn't we have path=None here?
There was a problem hiding this comment.
I thought a bit about this yesterday, but I'm not sure that have path=None is the right approach, as when using None that usually means that it is not needed in most cases or it defaults to some value within the code if not provided. This is not the case in plot_forecast as the value of "path" is related to the value of "output", so it's up to the user to define exactly what he wants. That's why I also changed the order as it makes more sense to me.
There was a problem hiding this comment.
I should have explained the issue better. I raised it when I wanted to plot in a jupyter notebook and I got an error for missing parameters. The fact that I had to add plot=None was counter-intuitive.
So the idea was to make path optional, which means adding path=None in the function signature, as @capelastegui suggests.
Edit: Changing the order of input parameters is a matter of style and not relevant in this issue.
There was a problem hiding this comment.
hm ok, I see. I'll proceed with the change then.
There was a problem hiding this comment.
Unless I'm mistaken, the purpose of #68 is to change the signature of plot_forecast to plot_forecast(df_fcast, output, path=None, ...) . Once we add that, I think everything is OK
|
@capelastegui @bezes guys could you please check my reply above and comment? As I'm seeing it, for me "path" is not exactly an optional parameter... |
| @@ -334,12 +333,13 @@ def plot_forecast(df_fcast, path, output='html', width=None, | |||
| | - date (timestamp) | |||
| | - model (str) : ID for the forecast model | |||
| | - y (float) : Value of the time series in that sample | |||
| | - is_actuals (bool) : True for actuals samples, False for forecasted samples # noqa | |||
| | - is_actuals (bool) : True for actuals samples, False for | |||
| | forecasted samples # noqa | |||
There was a problem hiding this comment.
you can remove #noqa here, it's for ignoring PEP8 long line warning
There was a problem hiding this comment.
I think we should raise a different issue to fix any docstrings that need fixing.
Codecov Report
@@ Coverage Diff @@
## master #71 +/- ##
==========================================
+ Coverage 84.85% 84.89% +0.04%
==========================================
Files 6 6
Lines 1373 1377 +4
==========================================
+ Hits 1165 1169 +4
Misses 208 208
Continue to review full report at Codecov.
|
anticipy/forecast_plot.py
Outdated
| @@ -375,6 +375,10 @@ def plot_forecast(df_fcast, path, output='html', width=None, | |||
| fig = _matplotlib_forecast_create(df_fcast, subplots, sources, | |||
| nrows, ncols, width, height, | |||
| title, dpi, show_legend) | |||
| if path is None: | |||
There was a problem hiding this comment.
This doesn't cover the empty string path=''. I think it's better to check if not path:
anticipy/forecast_plot.py
Outdated
| @@ -393,6 +397,10 @@ def plot_forecast(df_fcast, path, output='html', width=None, | |||
| fig = _plotly_forecast_create(df_fcast, subplots, sources, nrows, | |||
| ncols, width, height, title, | |||
| show_legend) | |||
| if path is None: | |||
…d relevant tests. #68
anticipy/forecast_plot.py
Outdated
| @@ -357,6 +357,10 @@ def plot_forecast(df_fcast, path, output='html', width=None, | |||
|
|
|||
| assert isinstance(df_fcast, pd.DataFrame) | |||
|
|
|||
| if not path and (output == 'html' or output == 'png'): | |||
| logger.error('No export path provided.') | |||
| return 0 | |||
There was a problem hiding this comment.
closes #68