Skip to content

Fix behavior of SARIMAXModel if simple_differencing=True is set #837

Merged
merged 23 commits into from
Aug 9, 2022

Conversation

Mr-Geekman
Copy link
Contributor

@Mr-Geekman Mr-Geekman commented Aug 4, 2022

Before submitting (must do checklist)

  • Did you read the contribution guide?
  • Did you update the docs? We use Numpy format for all the methods and classes.
  • Did you write any new necessary tests?
  • Did you update the CHANGELOG?

Proposed Changes

Look #836.

Closing issues

Closes #836.

@Mr-Geekman Mr-Geekman self-assigned this Aug 4, 2022
@github-actions
Copy link

github-actions bot commented Aug 4, 2022

🚀 Deployed on https://deploy-preview-837--etna-docs.netlify.app

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #837 (dd9978f) into master (c0a21a7) will increase coverage by 0.11%.
The diff coverage is 98.83%.

@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
+ Coverage   84.53%   84.64%   +0.11%     
==========================================
  Files         128      130       +2     
  Lines        7390     7450      +60     
==========================================
+ Hits         6247     6306      +59     
- Misses       1143     1144       +1     
Impacted Files Coverage Δ
etna/models/sarimax.py 93.65% <95.23%> (-0.47%) ⬇️
etna/libs/pmdarima_utils/__init__.py 100.00% <100.00%> (ø)
etna/libs/pmdarima_utils/arima.py 100.00% <100.00%> (ø)
etna/models/tbats.py 94.64% <100.00%> (ø)
etna/models/utils.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@github-actions github-actions bot temporarily deployed to pull request August 4, 2022 10:38 Inactive
@martins0n martins0n self-requested a review August 4, 2022 13:08
@github-actions github-actions bot temporarily deployed to pull request August 4, 2022 13:25 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 4, 2022 14:05 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 4, 2022 15:52 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 4, 2022 16:18 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 4, 2022 16:41 Inactive
@Mr-Geekman Mr-Geekman closed this Aug 5, 2022
@Mr-Geekman Mr-Geekman reopened this Aug 5, 2022
@github-actions github-actions bot temporarily deployed to pull request August 5, 2022 07:40 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 5, 2022 08:07 Inactive
d.a.bunin added 2 commits August 5, 2022 11:27
@github-actions github-actions bot temporarily deployed to pull request August 5, 2022 08:43 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 5, 2022 09:15 Inactive
)
end_idx = determine_num_steps(
start_timestamp=self._first_train_timestamp, end_timestamp=end_timestamp, freq=self._freq # type: ignore
)
Copy link
Contributor

Choose a reason for hiding this comment

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

May we should add assert there, something like
end_idx-start_idx == len(df), "Check that total number of steps to forecast is equal to total ts lenght and so so on

It's not obvious how number of steps has become index

def test_prediction_simple_differencing():
"""Check that SARIMAX gives similar results with different values of ``simple_differencing``.

We generate dataset from ``generate_ar_df`` with ``ar_coef=[1]`` and it gives us (0, 1, 1) process.
Copy link
Contributor

Choose a reason for hiding this comment

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

(1,0,1), no?

@github-actions github-actions bot temporarily deployed to pull request August 5, 2022 15:20 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 5, 2022 16:21 Inactive
Copy link
Contributor

@martins0n martins0n left a comment

Choose a reason for hiding this comment

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

👍

@github-actions github-actions bot temporarily deployed to pull request August 9, 2022 07:37 Inactive
@Mr-Geekman Mr-Geekman marked this pull request as draft August 9, 2022 07:50
@Mr-Geekman Mr-Geekman marked this pull request as ready for review August 9, 2022 08:05
@Mr-Geekman Mr-Geekman merged commit 7ef6231 into master Aug 9, 2022
@Mr-Geekman Mr-Geekman deleted the issue-836 branch August 9, 2022 08:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Fix behavior of SARIMAXModel if simple_differencing=True is set
3 participants