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

[Feature] Support a user defined function name in the window transformation output #1676

Merged

Conversation

JQGoh
Copy link
Contributor

@JQGoh JQGoh commented Mar 27, 2023

Summary

As mentioned in the darts' gitter chat, this PR supports a user-defined function name as part of the window transformation output name.

Without this feature, if we provide multiple user-defined functions for the window operations, we have multiple output names such as rolling_udf_5_target, rolling_udf_5_target_1 and etc. This poses the challenge in tracking the derived feature names.

Other Information

  • Also include an additional test case for window transformation that considers a user-defined function such as count_above_mean.

@JQGoh JQGoh requested a review from dennisbader as a code owner March 27, 2023 17:28
@JQGoh
Copy link
Contributor Author

JQGoh commented Mar 27, 2023

@madtoinou FYI

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.08 ⚠️

Comparison is base (87b2b69) 94.14% compared to head (fa8b144) 94.06%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1676      +/-   ##
==========================================
- Coverage   94.14%   94.06%   -0.08%     
==========================================
  Files         125      125              
  Lines       11389    11379      -10     
==========================================
- Hits        10722    10704      -18     
- Misses        667      675       +8     
Impacted Files Coverage Δ
.../dataprocessing/transformers/window_transformer.py 100.00% <ø> (ø)
darts/timeseries.py 92.66% <100.00%> (+0.14%) ⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JQGoh
Copy link
Contributor Author

JQGoh commented Mar 28, 2023

I realize that I did not validate that the function_name is a string type. Will do so after the code review.

Copy link
Collaborator

@madtoinou madtoinou left a comment

Choose a reason for hiding this comment

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

Looking good to me 🚀

I would just leave the "sum" transform name in the test, "TotalOperation" is less understandable in my opinion and we need to cover the case where the function name is not provided and fn is not "apply".

It's probably not necessary to check that function_name is a string, the documentation is clear and I don't see which other type the users might be tempted to use...

@JQGoh JQGoh force-pushed the feat/user-defined-fn-name-window-trans branch from 9993323 to aada4ac Compare March 28, 2023 09:09
Copy link
Collaborator

@madtoinou madtoinou left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments so quickly 🚀

@madtoinou madtoinou added this to In review in darts via automation Mar 29, 2023
@JQGoh
Copy link
Contributor Author

JQGoh commented Apr 4, 2023

@dennisbader May I check that any chance that this PR could be in time as part of the new release (0.23.2)? Thanks

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 good, thanks @JQGoh 🚀 And yes, this will be part of the 0.24.0 release :)

@dennisbader dennisbader merged commit e4908f7 into unit8co:master Apr 7, 2023
4 checks passed
darts automation moved this from In review to Done Apr 7, 2023
@dennisbader dennisbader moved this from Done to Released in darts May 23, 2023
alexcolpitts96 pushed a commit to alexcolpitts96/darts that referenced this pull request May 31, 2023
…mation output (unit8co#1676)

* Support user-defined name in window transformation

* Update doc string

* Add tests

* Review: Test respects the built-in function name

---------

Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
darts
Released
Development

Successfully merging this pull request may close these issues.

None yet

4 participants