Skip to content

Fix notebooks in inference track #974

Merged
merged 16 commits into from
Oct 10, 2022
Merged

Fix notebooks in inference track #974

merged 16 commits into from
Oct 10, 2022

Conversation

Mr-Geekman
Copy link
Contributor

@Mr-Geekman Mr-Geekman commented Oct 5, 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 #973.

Closing issues

Closes #973.

@Mr-Geekman Mr-Geekman self-assigned this Oct 5, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

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

@github-actions github-actions bot temporarily deployed to pull request October 5, 2022 11:36 Inactive
@martins0n martins0n self-requested a review October 6, 2022 07:37
@review-notebook-app
Copy link

review-notebook-app bot commented Oct 7, 2022

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-07T06:04:27Z
----------------------------------------------------------------

Line #6.    from etna.models.mixins import MultiSegmentModelMixin

Let's dont use these mixins, these are technical once for own use.

I guess we should make example from scratch without magic and multiple dependencies


Mr-Geekman commented on 2022-10-07T07:07:23Z
----------------------------------------------------------------

Even for per-segment type?

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.

comments in notebook

We should explain the purpose of our new context aware abastract classes, I think we miss it now

Copy link
Contributor Author

Even for per-segment type?


View entire conversation on ReviewNB

@github-actions github-actions bot temporarily deployed to pull request October 7, 2022 07:27 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 7, 2022 10:25 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 7, 2022 10:33 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 7, 2022 10:48 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

❗ No coverage uploaded for pull request base (inference@70c7b0b). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             inference     #974   +/-   ##
============================================
  Coverage             ?   85.24%           
============================================
  Files                ?      132           
  Lines                ?     7708           
  Branches             ?        0           
============================================
  Hits                 ?     6571           
  Misses               ?     1137           
  Partials             ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-07T11:58:29Z
----------------------------------------------------------------

These classes have different signatures for forecast and predict method depending on its name.

methods

theirs

if a model

It is an interval of dataset that goes right before the interval we want to make prediction on.

It is a part of a dataset before predection points that is necessary for making forecasts

predictions into the future - > seems like it's about any predicitions

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-07T11:58:30Z
----------------------------------------------------------------

Line #18.            )

What about kwargs and parameters saving for to_dict


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-07T11:58:30Z
----------------------------------------------------------------

Line #2.    

Why do we need this if we use pipeline below anyway?


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-07T11:58:31Z
----------------------------------------------------------------

predictions have been made - maybe


@review-notebook-app
Copy link

review-notebook-app bot commented Oct 7, 2022

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-07T11:58:32Z
----------------------------------------------------------------

Maybe we have the difference becouse of randomization?


Mr-Geekman commented on 2022-10-07T12:08:48Z
----------------------------------------------------------------

I will fix random seed inside model.

@@ -1 +1 @@
etna[all] @ git+https://github.com/tinkoff-ai/etna.git@master
etna[all] @ git+https://github.com/tinkoff-ai/etna.git@issue-974
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it, it is for the case when I want to run it on clean machine in binder (some warnings aren't present where).

Copy link
Contributor Author

I will fix random seed inside model.


View entire conversation on ReviewNB

@github-actions github-actions bot temporarily deployed to pull request October 7, 2022 12:42 Inactive
@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

martins0n commented on 2022-10-10T11:27:30Z
----------------------------------------------------------------

Line #7.    log = LogTransform(in_column="target", out_column="log")

We don't use it in transforms


@github-actions github-actions bot temporarily deployed to pull request October 10, 2022 12:37 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 10, 2022 13:03 Inactive
@Mr-Geekman Mr-Geekman merged commit 8334d2d into inference Oct 10, 2022
@Mr-Geekman Mr-Geekman mentioned this pull request Oct 10, 2022
@Mr-Geekman Mr-Geekman mentioned this pull request Oct 26, 2022
4 tasks
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.

3 participants