Skip to content

Closes #290 #313

Merged
merged 3 commits into from
Nov 27, 2021
Merged

Closes #290 #313

merged 3 commits into from
Nov 27, 2021

Conversation

Carlosbogo
Copy link
Contributor

Renames parameters for number of folds in different functions to keep naming consistency

Carlosbogo and others added 2 commits November 23, 2021 10:38
@Carlosbogo
Closes tinkoff-ai#290
d01fa6f 
Renames parameters for number of folds
in different functions to keep consistency
@Carlosbogo
Merge branch 'tinkoff-ai:master' into master
db21ebe
@iKintosh
Copy link
Contributor

As I can see you didn't change tests, that's why test workflow fails: https://github.com/tinkoff-ai/etna/runs/4313774890?check_suite_focus=true#step:8:920
You may run tests locally to catch all errors.

iKintosh
iKintosh suggested changes Nov 24, 2021
View reviewed changes
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.

Also need changes to tests.

@iKintosh iKintosh linked an issue Nov 24, 2021 that may be closed by this pull request
Rename cv to n_folds #290
Closed
1 task
@Carlosbogo
Copy link
Contributor Author

Sorry, but I don't quite understand. Could you give me an example on what I need to change in the tests so that they work properly?

@iKintosh
Copy link
Contributor

iKintosh commented Nov 26, 2021 •
edited
Loading

Sure. Happy to do that!

So you have changed pipeline and stacking ensemble classes. You renamed cv to n_folds. But we are testing this functionality in several tests.

As you can see, here we try to call stacking ensemble class with several cv parameters, however now it is called n_folds:
https://github.com/tinkoff-ai/etna/blob/master/tests/test_ensembles/test_stacking_ensemble.py#L41

So in order to make it it work again you should change cv to n_folds in tests as well.

In the example above it means changing

ensemble = StackingEnsemble(pipelines=[naive_pipeline_1, naive_pipeline_2], cv=input_cv)

To

ensemble = StackingEnsemble(pipelines=[naive_pipeline_1, naive_pipeline_2], n_folds=input_cv)

You can find all test that are testing stacking ensemble class here https://github.com/tinkoff-ai/etna/blob/master/tests/test_ensembles/test_stacking_ensemble.py
And all tests that are testing pipeline class here https://github.com/tinkoff-ai/etna/tree/master/tests/test_pipeline

Carlosbogo added a commit to Carlosbogo/etna that referenced this pull request Nov 27, 2021
@Carlosbogo
Changes tests to keep consistency with tinkoff-ai#313
381aeb3 
Fixes tinkoff-ai#313 to close tinkoff-ai#290
@Carlosbogo Carlosbogo mentioned this pull request Nov 27, 2021
Closed
@Carlosbogo
Squashed commit of the following:
b6210f0 
commit 381aeb3
Author: Carlosbg <bogomcar@gmail.com>
Date:   Sat Nov 27 14:23:16 2021 +0100

    Changes tests to keep consistency with tinkoff-ai#313

    Fixes tinkoff-ai#313 to close tinkoff-ai#290

commit c074d3b
Author: Andrey Alekseev <ilekseev@gmail.com>
Date:   Fri Nov 26 15:28:12 2021 +0300

    add acf plot; change eda notebook; (tinkoff-ai#318)

    * add acf plot; change eda notebook;

    * add changed to changelog

    Co-authored-by: an.alekseev <an.alekseev@tinkoff.ru>

commit 38623dc
Author: Mr-Geekman <36005824+Mr-Geekman@users.noreply.github.com>
Date:   Thu Nov 25 19:44:42 2021 +0300

    Add `duplicate_data` (tinkoff-ai#305)

    * Add utils file, function , tests for it

    * Add example for

    * Update changelog

    * Correct typos in docstring

    * Change default value for duplicate_data

commit c2070a1
Author: Andrey Alekseev <ilekseev@gmail.com>
Date:   Thu Nov 25 19:44:17 2021 +0300

    add inverse transform as final step in forecast method; also rephrase… (tinkoff-ai#316)

    * add inverse transform as final step in forecast method; also rephrase _validate_backtest_dataset docstring

    * add inverse transform as final step in fit method; change test; change example

    Co-authored-by: an.alekseev <an.alekseev@tinkoff.ru>

commit e66058f
Author: Andrey Alekseev <ilekseev@gmail.com>
Date:   Wed Nov 24 18:13:03 2021 +0300

    Parsing type hints in Sphinx documentation (tinkoff-ai#205)

    * update sphinx in order to parse type hints; make flake8-docstyle numpydocstyle compatible
    * update deps

commit e814219
Author: Martin Gabdushev <33594071+martins0n@users.noreply.github.com>
Date:   Wed Nov 24 18:10:04 2021 +0300

    :bomb: release 1.3.3 (tinkoff-ai#312)
iKintosh
iKintosh approved these changes Nov 27, 2021
View reviewed changes
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.

Thanks!
🎉 🚀

@codecov-commenter
Copy link

Codecov Report

Merging #313 (b6210f0) into master (0ec3638) will decrease coverage by 33.32%.
The diff coverage is 34.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #313       +/-   ##
===========================================
- Coverage   87.75%   54.42%   -33.33%     
===========================================
  Files          96       97        +1     
  Lines        4737     4777       +40     
===========================================
- Hits         4157     2600     -1557     
- Misses        580     2177     +1597
Impacted Files Coverage Δ
etna/analysis/outliers/median_outliers.py 25.00% <ø> (-70.84%) ⬇️
etna/analysis/plotters.py 12.43% <ø> (ø)
etna/datasets/tsdataset.py 71.07% <ø> (-21.49%) ⬇️
etna/metrics/metrics.py 87.09% <ø> (-12.91%) ⬇️
etna/transforms/special_days.py 28.75% <ø> (-70.00%) ⬇️
etna/analysis/eda_utils.py 20.00% <7.14%> (-2.23%) ⬇️
etna/datasets/utils.py 34.78% <34.78%> (ø)
etna/pipeline/pipeline.py 85.39% <66.66%> (-14.05%) ⬇️
etna/analysis/__init__.py 100.00% <100.00%> (ø)
etna/datasets/__init__.py 100.00% <100.00%> (ø)
... and 67 more

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 0ec3638...b6210f0. Read the comment docs.

@iKintosh iKintosh merged commit a764443 into tinkoff-ai:master Nov 27, 2021
@Carlosbogo
Copy link
Contributor Author

Thank you for your feedback and for guiding me through the PR process! I hope it helps.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iKintosh iKintosh iKintosh approved these changes

@alex-hse-repository alex-hse-repository Awaiting requested review from alex-hse-repository

Assignees

@Carlosbogo Carlosbogo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename cv to n_folds
3 participants
@Carlosbogo @iKintosh @codecov-commenter