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

weighted moving average #122

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

shubhamnaikk
Copy link

Description

This update addresses discrepancies in the Weighted Moving Average (WMA) functionality and its corresponding test cases.

Key Changes

Corrected Unit Test Assertions:
Updated the expected values in the test cases based on manual recalculations of the WMA formula.
Ensured the expected results match the implementation for both default linear weights and custom weights.
Validated WMA Implementation:
Verified the correctness of the WMA calculation for different scenarios, including grouped and ungrouped data.
Tested with varying window sizes and weights.
Improved Test Coverage:
Ensured tests cover edge cases, such as insufficient data for rolling calculations.
Included assertions for grouped and non-grouped datasets.

Outcomes

The Weighted Moving Average functionality is now thoroughly validated and aligned with the expected behavior.
All tests pass successfully, confirming the correctness of the implementation.
This update strengthens the reliability of the WMA feature in the data transformation module.

@shubhamnaikk shubhamnaikk requested a review from a team as a code owner November 25, 2024 01:13
Copy link
Member

@dannymeijer dannymeijer 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 your contribution @shubhamnaikk , really appreciate it.

A few points:

  • The placement of your modules does not quite fit with how we intend, it would be better if we placed these elsewhere;
  • The way you wrote the code, it is not making use of the Koheesio framework (Step classes) along with the typing that we intend to enforce.
  • Currently we have no module for pure python implementations (Transformation in your case) - we should add that as part of your PR if that is the intended use.

Also, can you explain the intended use for this a bit more? Would this be for ML use-cases with the input being something like pandas perhaps? Or did you have something else in mind.

I propose that we have a small meetup to discuss, as I would love to add your contribution to our library. Please reach out to me in a DM / email so we can discuss further.

@dannymeijer
Copy link
Member

Please also see: #129

@dannymeijer
Copy link
Member

There has not been any response for the last 2 weeks. Please respond or address the aformentioned concerns before the end of the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants