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

fix TimeSeries.pd_series not retaining original name #2102

Conversation

fmerinocasallo
Copy link
Contributor

@fmerinocasallo fmerinocasallo commented Nov 30, 2023

Fixes #2101.

Summary

In this new version of the TimeSeries.pd_series method, the newly created Panda.Series instance to be returned takes as its name the one from the TimeSeries object (stored as the first item in the iterable attribute self.components).

Other Information

This new version of the TimeSeries.pd_series method returns a new Pandas.Series instance with the name of the original TimeSeries object (series).

>>> import pandas as pd
>>> from darts.timeseries import TimeSeries
>>> idx = pd.date_range("2018-01-01", periods=5, freq="D")
>>> ts = pd.Series(range(len(idx)), index=idx, name="series_name")
>>> ts
2018-01-01    0
2018-01-02    1
2018-01-03    2
2018-01-04    3
2018-01-05    4
Freq: D, Name: series_name, dtype: int64
>>> series = TimeSeries.from_series(ts)
>>> series.pd_series()
time
2018-01-01    0.0
2018-01-02    1.0
2018-01-03    2.0
2018-01-04    3.0
2018-01-05    4.0
Freq: D, Name: series_name, dtype: float64

By directly accessing to the `components.array` attribute from the TimeSeries
instance, I do not have to convert the `components` attribute to a list
anymore.
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Hi @fmerinocasallo, thanks for this fix and the well structured issue 🚀

Just a minor suggestion, then we can merge :)

darts/timeseries.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
As suggested by @dennisbader, we can directly access the items in the `components` attribute from the TimeSeries instance, no need to use the `components.array` attribute.

Co-authored-by: Dennis Bader <[email protected]>
@fmerinocasallo
Copy link
Contributor Author

fmerinocasallo commented Nov 30, 2023

Hey @dennisbader, thanks for your suggestion! 😄 You are absolutely right. For some unknown reason, I assumed we could not directly access items in the components attribute from TimeSeries instances 😅 I have already commited your suggested changes 👌

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Looks great now, thanks @fmerinocasallo 🚀

@dennisbader
Copy link
Collaborator

You can reformat the code with black to resolve the failing lint check.

Also if you plan on contributing more in the future, you could also set up the pre-commit hooks described in our CONTRIBUTING.md that will automatically check and reformat the code

@fmerinocasallo
Copy link
Contributor Author

I reformatted the code with black to resolve the failing link check, as instructed. However, there are a couple of tests that are still not successful. Is there something else I can do on my side to resolve this?

@dennisbader
Copy link
Collaborator

Hi @fmerinocasallo, yes there is one unit test failing. This is usually expected when changing something. Can you try to fix it? If you need help let us know.

@fmerinocasallo
Copy link
Contributor Author

fmerinocasallo commented Dec 5, 2023

I am sorry to bother you again, but I am afraid I could use some help to fix that unit test failing, as I do not know how to proceed 😅 I would appreciate if you continue pointing me on the right direction 🙏 This way, I can learn to fix it by myself and do it autonomously next time.

@dennisbader
Copy link
Collaborator

dennisbader commented Dec 7, 2023

For sure. Generally when contributing, you can set up your dev environment as described here (points 1 - 6).

For testing, you can run all unit tests from bash and the darts repository root (or just use your IDE):

pytest . --no-cov

For a specific file, you can run (that is the file which has the failing test):

pytest darts/tests/test_timeseries_static_covariates.py --no-cov

You can inspect also the failing tests here on github actions.

Then, try to analyze what's going wrong in the test (darts/tests/test_timeseries_static_covariates.py).

  • you can adapt the test itself if only the expected output should change
  • you should adapt your new logic, if the test fails because of some unexpected reason

Hope this helps

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Hi @fmerinocasallo and thanks again for the contribution. 🚀 💯
I pushed to fix to the failing unit test, since we want to make a patch release soon.

If you want, you can check out what I did in my latest commits to see how I normally do this (in case you want to contribute more in the future :) ).

@dennisbader dennisbader merged commit 8897e93 into unit8co:master Dec 10, 2023
8 of 9 checks passed
@fmerinocasallo
Copy link
Contributor Author

Thanks for your patience and assistance, @dennisbader :) I am sorry for my slow progress on this front 😓

I have checked out your latest commits and understood the issue regarding the row indices of static cov dataframes I was struggling with. I thought the series.quantile_timeseries() was supposed to add the string literal "_quantiles" to component names. Now I realize "_quantiles" was a placeholder (as in "_{quantiles}") 😅

I will keep an eye on the project in case I can continue to contribute in the future 😉

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.

[BUG] TimeSeries.pd_series does not retain original name
2 participants