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

add TimesSeries.from_group_dataframe parallel mode #2292

Conversation

BohdanBilonoh
Copy link
Contributor

Checklist before merging this PR:

  • Mentioned all issues that this PR fixes or addresses.
  • Summarized the updates of this PR under Summary.
  • Added an entry under Unreleased in the Changelog.

Summary

TimeSeries.from_group_dataframe creates time series in for loop. It is suboptimal if the number of groups is large or very large.

darts/timeseries.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
@BohdanBilonoh BohdanBilonoh force-pushed the refactor/parallel-from-groud-dataframe branch from 229e095 to 5ef59c2 Compare March 20, 2024 09:34
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.

Thanks @BohdanBilonoh for this neat PR 🚀
This will be a great improvement when dealing with a large number of time series groups :)

I had some minor suggestions on how we could potentially simplify some things here and there.

Also, we're soon to merge PR #2284 which separates the TimeSeries specific utils (will be moved into a new ts_utils.py) from the non-specific utils.

After that, you can import _parallel_apply and _build_tqdm_iterator in timeseries.py, and also import them in other places as before. We can then remove these changes from this PR.

So I'd say let's ignore my comment about this for now and let's address the other comments first. After we've merged the other PR, we can apply the final changes here.

darts/utils/__init__.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
darts/tests/test_timeseries.py Outdated Show resolved Hide resolved
@dennisbader
Copy link
Collaborator

Thanks for already applying the changes @BohdanBilonoh, it looks great now! 🚀
I'll reach out to you once we've merge the other PR. I can then also apply the changes myself.

@BohdanBilonoh
Copy link
Contributor Author

Thanks for already applying the changes @BohdanBilonoh, it looks great now! 🚀 I'll reach out to you once we've merge the other PR. I can then also apply the changes myself.

Thanks for review

@dennisbader
Copy link
Collaborator

#2284 was merged, so we can apply the changes here now 🚀

@BohdanBilonoh BohdanBilonoh force-pushed the refactor/parallel-from-groud-dataframe branch from ccf0330 to a0ce9f3 Compare April 8, 2024 18:45
@BohdanBilonoh BohdanBilonoh force-pushed the refactor/parallel-from-groud-dataframe branch from 945ef3f to 4ea28f0 Compare April 8, 2024 18:49
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.

Thanks @BohdanBilonoh, for applying the last changes. Great job, this is exactly how I imagined it 🚀 🚀 🚀

@dennisbader dennisbader merged commit e50854b into unit8co:master Apr 9, 2024
7 of 9 checks passed
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.

None yet

4 participants