Skip to content

Change ProphetModel and SARIMAXModel according to latest architecture #549

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

alex-hse-repository
Copy link
Collaborator

@alex-hse-repository alex-hse-repository commented Feb 18, 2022

IMPORTANT: Please do not create a Pull Request without creating an issue first.

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?

Type of Change

  • Examples / docs / tutorials / contributors update
  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Proposed Changes

Related Issue

Closing issues

closes #507

@alex-hse-repository alex-hse-repository added the enhancement New feature or request label Feb 18, 2022
@alex-hse-repository alex-hse-repository self-assigned this Feb 18, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2022

Codecov Report

Merging #549 (fe8d195) into master (2242d40) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
+ Coverage   87.26%   87.33%   +0.06%     
==========================================
  Files         118      118              
  Lines        5788     5700      -88     
==========================================
- Hits         5051     4978      -73     
+ Misses        737      722      -15     
Impacted Files Coverage Δ
etna/models/catboost.py 100.00% <ø> (ø)
etna/models/sklearn.py 96.07% <ø> (ø)
etna/models/base.py 87.59% <100.00%> (+10.15%) ⬆️
etna/models/holt_winters.py 98.78% <100.00%> (+0.01%) ⬆️
etna/models/prophet.py 98.76% <100.00%> (+0.37%) ⬆️
etna/models/sarimax.py 93.85% <100.00%> (-1.69%) ⬇️
etna/models/seasonal_ma.py 97.14% <100.00%> (+0.08%) ⬆️
etna/pipeline/pipeline.py 97.39% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2242d40...fe8d195. Read the comment docs.

Copy link
Contributor

@iKintosh iKintosh left a comment

Choose a reason for hiding this comment

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

Looks a lot cleaner!

Almost ready to merge. Just need some clarification on SARIMAXAdapter

Comment on lines +237 to +238
if isinstance(segment_predict, np.ndarray):
segment_predict = pd.DataFrame({"target": segment_predict})
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +278 to +283
y_pred = y_pred.reset_index(drop=True, inplace=False)
rename_dict = {
column: column.replace("mean", "target") for column in y_pred.columns if column.startswith("mean")
}
y_pred = y_pred.rename(rename_dict, axis=1)
return y_pred
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor

@iKintosh iKintosh left a comment

Choose a reason for hiding this comment

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

🔥

@alex-hse-repository alex-hse-repository merged commit 8f594cf into master Feb 18, 2022
@iKintosh iKintosh deleted the issue-507 branch March 22, 2022 08:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change ProphetModel and SARIMAXModel according to latest architecture
3 participants