[BUG] Something went wrong with multi-index if backtest (issue #461) #771
Conversation
|
I did not update changlog and tests as I am not sure, that I corrected all the issues in the bug-report |
mvakhmenin
changed the title
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
=======================================
Coverage 83.81% 83.81%
=======================================
Files 123 123
Lines 6895 6896 +1
=======================================
+ Hits 5779 5780 +1
Misses 1116 1116
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| @@ -456,6 +456,7 @@ def _get_backtest_forecasts(self) -> pd.DataFrame: | |||
| forecast = forecast.join(fold_number_df) | |||
| forecasts_list.append(forecast) | |||
| forecasts = pd.concat(forecasts_list) | |||
| forecasts.sort_index(axis="columns", ascending=[True, False], inplace=True) | |||
| return forecasts | |||
There was a problem hiding this comment.
We dont need specify ascending order here.
There was a problem hiding this comment.
@martins0n
Checks failed with the following error. I cannot understand, what should I do
ImportError while loading conftest '/home/runner/work/etna/etna/tests/test_ensembles/conftest.py'. tests/test_ensembles/conftest.py:16: in <module> from etna.models import ProphetModel E ImportError: cannot import name 'ProphetModel' from 'etna.models' (/home/runner/work/etna/etna/etna/models/__init__.py) Error: Process completed with exit code 4.
There was a problem hiding this comment.
It's ok, will fix it today or next day
You can make futher changes and test locally, via command poetry run pytest tests -v --cov=etna -m "not long" --cov-report=xml and run linters via make lint
There was a problem hiding this comment.
@mvakhmenin We've merged updates to master branch. You can rebase on updated master.
There was a problem hiding this comment.
@martins0n
Tests fail without ascending order
There was a problem hiding this comment.
@mvakhmenin We don't expect such ordering anyway, it've just happened by chanse.
it's better to change fixtures in conftest to make tests pass
There was a problem hiding this comment.
@martins0n
I do not understand, what's the problem with lint
There was a problem hiding this comment.
You can run linters locally and fix issues with command: make format and make lint
Current error could be fixed with make format I guess.
There was a problem hiding this comment.
You can find more information about contributing in guide
tests/test_pipeline/conftest.py
Outdated
| @@ -168,7 +168,7 @@ def step_ts() -> Tuple[TSDataset, pd.DataFrame, pd.DataFrame]: | |||
| forecast_df.columns = pd.MultiIndex.from_product( | |||
| [[segment], ["target", "fold_number"]], names=("segment", "feature") | |||
| ) | |||
|
|
|||
| forecast_df.sort_index(axis=1, inplace=True) | |||
There was a problem hiding this comment.
Can we fix it above at 168 line by hand changing target and fold_number to make it more determinitic?
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Before submitting (must do checklist)
Type of Change
Proposed Changes
Related Issue
Closing issues
closes #461