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.

Sign up for GitHub

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

Jump to bottom

[BUG] resample_time_stamps fails when time_stamps is int and max_forecast_steps is None #28

Closed
datenzauberai opened this issue Oct 11, 2021 · 2 comments
Closed

[BUG] resample_time_stamps fails when time_stamps is int and max_forecast_steps is None #28

datenzauberai opened this issue Oct 11, 2021 · 2 comments

Comments

@datenzauberai
Copy link

datenzauberai commented Oct 11, 2021 •
edited

Describe the bug
Calling resample_time_stamps fails when requesting an integer number of time_stamps and max_forecast_steps has not been set (code tries to subset time_stamps then which won't work for integers).

This can be solved by setting the variable tf later which is only needed when a list of time_stamps has been passed.

To Reproduce
This will fail...

data = pd.Series([100, 100, 100, 100])
model = Arima(ArimaConfig(order=(1, 1, 0)))
model.train(TimeSeries.from_pd(data))
model.forecast(time_stamps=1)

This works...

data = pd.Series([100, 100, 100, 100])
model = Arima(ArimaConfig(order=(1, 1, 0), max_forecast_steps=42))
model.train(TimeSeries.from_pd(data))
model.forecast(time_stamps=1)

Expected behavior

Passing time_stamps as int should work when max_forecast_steps has not been set.

Desktop (please complete the following information):

  • Merlion: 1.0.0
@aadyotb
Copy link
Contributor

aadyotb commented Oct 11, 2021

Thanks for exposing this issue. There is a logic error in how resample_time_stamps handled integer values of time_stamps. This is pretty straightforward to address, and I'll get to it soon.

aadyotb added a commit that referenced this issue Oct 11, 2021
@aadyotb
Address Issue #28.
8907257 
Fix the handling of integer # of timestamps in resample_time_stamps.
This was referenced Oct 11, 2021
Closed
Merged
paulkass pushed a commit that referenced this issue Oct 18, 2021
@aadyotb
Address issues #27, #28, #29, #34 about timestamps & sampling granula…
50583be 
…rities. (#30)

* Address Issue #28.

Fix the handling of integer # of timestamps in resample_time_stamps.

* Address Issue #27.

Make SARIMA resample to default seasonality, instead of hourly.

* Admit more general granularities (Issue #29).

* Stop git checkout from failing w/ local changes.

* Update forecaster base docs.

* More sensible helper fn name.

* Remove commits from build_docs.sh.

* Convert timedelta & last_train_time to pandas.

* Update version.

* Remove unused method.

* Make AutoProphet robust to monthly granularity.

* Remove commented out line.

* Handle more freqs in TimeSeries constructor.

Addresses Issue #34.
@aadyotb
Copy link
Contributor

aadyotb commented Oct 18, 2021

Closing this issue as it has been addressed by PR #30.

@aadyotb aadyotb closed this as completed Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@datenzauberai @aadyotb