Closed narencastellon closed 1 year ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Hey @Naren8520, can you please rename this notebook to use underscores (prediction_intervals_in_forecasting_models), move it to the nbs/docs
folder and add it to the nbs/sidebar.yml
file?
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:26Z ----------------------------------------------------------------
This should be "Installing mlforecast"
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:27Z ----------------------------------------------------------------
I think this can be deleted
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:28Z ----------------------------------------------------------------
We don't have statsmodels as a dependency so this will likely fail if it was tested. You can add the statsmodels library to the environment.yml file
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:29Z ----------------------------------------------------------------
plotly is not used so please remove those imports. also the rcParams is available in matplotlib, so you could use plt.rcParams instead (to remove the pylab import)
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:29Z ----------------------------------------------------------------
This isn't exactly true. We need those three columns but they can have other names. Also the ds can be an integer.
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:30Z ----------------------------------------------------------------
You can avoid this by using parse_dates=['ds']
in the read command above
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:31Z ----------------------------------------------------------------
You can delete this cell as well if you use the parse_dates
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:32Z ----------------------------------------------------------------
Please use utilsforecast.plotting.plot_series
here.
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:33Z ----------------------------------------------------------------
It's preferable to use snake_case everywhere, so please rename this to augmented_dickey_fuller_test
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:33Z ----------------------------------------------------------------
Line #1. from statsmodels.tsa.seasonal import seasonal_decompose
please move this import to the start of the notebook
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:34Z ----------------------------------------------------------------
Line #1. from statsmodels.tsa.seasonal import seasonal_decompose
remove this import
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:35Z ----------------------------------------------------------------
This can be achieved with utilsforecast.plotting.plot_series(train, test)
Naren8520 commented on 2023-09-07T20:23:08Z ----------------------------------------------------------------
I think the function does not display both sets, or if it displays it, it cannot be seen because of the color
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:36Z ----------------------------------------------------------------
I think this could be renamed to something like "Modeling with MLForecast"
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:37Z ----------------------------------------------------------------
Please move these imports to the start of the notebook as well, it makes it easier to see which dependencies are needed.
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:38Z ----------------------------------------------------------------
Please move these imports to the start of the notebook as well, it makes it easier to see which dependencies are needed.
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:39Z ----------------------------------------------------------------
Line #2. ]
Move this bracket to the previous line
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:39Z ----------------------------------------------------------------
If the data frequency is 30 min the first difference would subtract the value from the previous 30 min, not the previous day. Also in the plot above there doesn't seem to be a trend.
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:40Z ----------------------------------------------------------------
Line #2. freq='30min', # Dayly
Remove the comment, the frequency isn't daily
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:41Z ----------------------------------------------------------------
This function isn't used in the model, either remove it or use it
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:42Z ----------------------------------------------------------------
There's no need to assign it back, so you can just use mlf.fit, also please put each argument in a separate line so it's easier to see them. Since the column names are the default ones you can remove those arguments as well (id_col, time_col, target_col)
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:43Z ----------------------------------------------------------------
These should be in the same plot. If you remove the differences argument (which I don't think we need in this example) they will be on the same scale. Also please use matplotlib (we don't have seaborn listed as a dependency)
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:44Z ----------------------------------------------------------------
This should be something like "Producing prediction intervals" given that the intervals aren't confidence intervals
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:44Z ----------------------------------------------------------------
Line #2. preds = mlf.predict(h=30, level=levels)#
Remove the comment at the end
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:45Z ----------------------------------------------------------------
Delete this cell, it's the same as the one below
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:46Z ----------------------------------------------------------------
Delete this cell
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:47Z ----------------------------------------------------------------
use the plot_series function here as well
View / edit / reply to this conversation on ReviewNB
jmoralez commented on 2023-09-06T22:14:48Z ----------------------------------------------------------------
The Nixtla Parameters points to the arch model. I think we can delete that one.
I think the function does not display both sets, or if it displays it, it cannot be seen because of the color
View entire conversation on ReviewNB
Description
Checklist: