Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename TSDataset.index -> TSDataset.timestamps, TSDataset.add_columns_from_pandas -> TSDataset.add_features_from_pandas, TSDataset.update_columns_from_pandas -> TSDataset.update_features_from_pandas #593

Merged
merged 10 commits into from
Feb 3, 2025

Conversation

ArtemLiA
Copy link

@ArtemLiA ArtemLiA commented Jan 31, 2025

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

Rename TSDataset.index to TSDataset.timestamps and make it read-only:

  • Rename TSDataset.index to TSDataset.timestamps
  • Make it return self.df.index.copy() (make read-only)
  • Fix all places where ts.index was used and replace them with ts.timestamps

Rename TSDataset.add_columns_from_pandas to TSDataset.add_features_from_pandas:

  • Rename TSDataset.add_columns_from_pandas to TSDataset.add_features_from_pandas
  • Replace all usage of old name
  • Fix tests

Rename TSDataset.update_columns_from_pandas to TSDataset.update_features_from_pandas:

  • Rename TSDataset.update_columns_from_pandas to TSDataset.update_features_from_pandas
  • Replace all usage of old name
  • Fix tests and fixtures

Closing issues

Closes #564 #567

* Rename `TSDataset.index` to `TSDataset.timestamps`
* Make it return `self.df.index.copy()` (make read-only)
* Fix all places where `ts.index` was used and replace them with `ts.timestamps`
…_from_pandas`

* Rename `TSDataset.add_columns_from_pandas` to `TSDataset.add_features_from_pandas`
* Replace all usage of old name
* Fix tests
…atures_from_pandas`

* Rename `TSDataset.update_columns_from_pandas` to `TSDataset.update_features_from_pandas`
* Replace all usage of old name
* Fix tests and fixtures
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

CHANGELOG.md Outdated Show resolved Hide resolved
@ArtemLiA
Copy link
Author

ArtemLiA commented Jan 31, 2025

Hi! I have reproduced failed test test_ensembles/test_stacking_ensemble::test_make_features. The traceback looks like this:

  File "/home/artem_liakhov/Projects/etna/tests/test_ensembles/conftest.py", line 214, in targets
    [
  File "/home/artem_liakhov/Projects/etna/tests/test_ensembles/conftest.py", line 215, in <listcomp>
    example_tsds[forecasts_ts[0].timestamps.min() : forecasts_ts[0].timestamps.max(), segment, "target"]
  File "/home/artem_liakhov/Projects/etna/.venv/lib/python3.10/site-packages/pandas/core/generic.py", line 5902, in __getattr__
    return object.__getattribute__(self, name)
AttributeError: 'DataFrame' object has no attribute 'timestamps'. Did you mean: 'to_timestamp'?

but according to the type annotation forecast_ts has type List[TSDataset] and the fixture of the same name returns the List[TSDataset]. Should this be a bug? Do you have any idea which test might be having problems and how to fix it?

@d-a-bunin
Copy link
Collaborator

Hi! It is probaby the same problem as in #563. Look at the comments. The probable solution can be seen here.

@ArtemLiA
Copy link
Author

ArtemLiA commented Feb 1, 2025

Thanks! I found the reason. Error was in typing annotation of forecasts_ts fixtures. It returns and should return List[pd.DataFrame] instead of List[TSDataset].

I fixed annotation and renamed forecasts_ts to forecasts_df so that name doesn't mislead anyone in the future.

Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.59%. Comparing base (307f635) to head (354ad22).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
etna/analysis/eda/plots.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   90.59%   90.59%           
=======================================
  Files         261      261           
  Lines       18061    18061           
=======================================
  Hits        16363    16363           
  Misses       1698     1698           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ArtemLiA ArtemLiA requested a review from d-a-bunin February 1, 2025 13:31
Copy link
Collaborator

@d-a-bunin d-a-bunin left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename method TSDataset.index -> TSDataset.timetamps and make it read-only
2 participants