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.

Sign up for GitHub

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

Jump to bottom

380: Initial implementation of moving_product operation #383

Merged
merged 11 commits into from Mar 7, 2024
Merged

380: Initial implementation of moving_product operation #383

merged 11 commits into from Mar 7, 2024

Conversation

akshatvishu
Copy link
Contributor

@akshatvishu akshatvishu commented Feb 28, 2024 •
edited

Closes #380

Implemented initial setup for moving_product in Temporian. Added method stubs and began C++ integration.

ToDo:

  • Add test
  • Add docs
  • Run Code Formatter (Black)
  • [ ]Maybe add it under temporian/temporian/beam

@akshatvishu
WIP: Initial implementation of moving_product operation
e03ecae
@akshatvishu
Remove the wrong use of override keyword at Reset Method in the Movin…
3c42893 
…gProductAccumulator class
@akshatvishu
Implement On-Demand Product Calculation in MovingProductAccumulator
31b197a 
Key Changes:
- Start and end indices (`start_idx` and `end_idx`) introduced to track the window.
- `Add` and `Remove` methods updated to adjust the window indices without affecting the product.
- `Result` method now calculates the product on-demand, considering zeros and ignoring NaN values for accuracy.
- Added a TODO comment to explore future optimizations for the Result method to enhance calculation efficiency.
@javiber javiber changed the title WIP: Initial implementation of moving_product operation 380: [WIP] Initial implementation of moving_product operation Mar 4, 2024
@javiber javiber changed the title 380: [WIP] Initial implementation of moving_product operation 380: Initial implementation of moving_product operation Mar 4, 2024
@javiber
Copy link
Collaborator

javiber commented Mar 4, 2024

Hi @akshatvishu I just tested this locally and found a couple of small things:

  • The example in event_set_ops.py line 3200 fails because the result is expressed in exponential notation due to the size of the results. Adjust the notation or use smaller values in the input.
  • Add the cumprod operation as a moving_prod with an infinite window length (cumsum for reference). As you can guess from the implementation, this is going to be pretty slow so add some warnings on the documentation and we will add a todo to optimize it in the future.
  • In the do you mention that "If the window does not contain any non-missing, non-zero values, the output for that window is missing (NaN)." I'd like a test checking that this is the case.

Other than these small things the PR is looking great, I think we can merge this after these points are addressed.

javiber
javiber reviewed Mar 7, 2024
View reviewed changes
temporian/core/event_set_ops.py Outdated Show resolved Hide resolved
temporian/core/event_set_ops.py Outdated Show resolved Hide resolved
temporian/core/event_set_ops.py Show resolved Hide resolved
@javiber javiber merged commit 9ccd08f into google:main Mar 7, 2024
13 of 14 checks passed
@ianspektor
Copy link
Collaborator

Congrats and thanks on the huge contribution @akshatvishu! 💪🏼

@akshatvishu
Copy link
Contributor Author

Congrats and thanks on the huge contribution @akshatvishu! 💪🏼

Big thanks to @javiber for his blazing fast reviews and suggestions. Couldn't have done this without his guidance 🙏

@akshatvishu akshatvishu deleted the moving_product branch March 8, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiber javiber javiber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New operator: moving_product
3 participants
@akshatvishu @javiber @ianspektor