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 aggregate function #232

Merged
merged 6 commits into from
Oct 3, 2023
Merged

fix aggregate function #232

merged 6 commits into from
Oct 3, 2023

Conversation

jmoralez
Copy link
Member

@jmoralez jmoralez commented Sep 13, 2023

  • Changes the aggregate function to handle both balanced and unbalanced series in the same way
  • Deprecates the is_balanced argument
  • Adds a simple test case that fails in the current main branch

Fixes #211
Fixes #224

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jmoralez jmoralez marked this pull request as ready for review September 14, 2023 21:54
@jmoralez jmoralez requested a review from AzulGarza September 14, 2023 21:54
@jmoralez jmoralez changed the title keep all unique_ids in aggregate Fix aggregate function Sep 14, 2023
@jmoralez jmoralez added the fix label Sep 14, 2023
@jmoralez jmoralez changed the title Fix aggregate function fix aggregate function Sep 14, 2023
@candalfigomoro
Copy link

@jmoralez I think the is_balanced parameter was never included in a released version of the library (just the main branch on Git), so if you want to remove it, maybe you can just remove it instead of deprecating it.

@jmoralez
Copy link
Member Author

@candalfigomoro it's in 0.3.0.

@candalfigomoro
Copy link

@candalfigomoro it's in 0.3.0.

@jmoralez You are right, I thought it was introduced with commit c107217 after version 0.3.0 but that commit only extended its usage.

Copy link
Member

@AzulGarza AzulGarza left a comment

Choose a reason for hiding this comment

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

Thanks for this @jmoralez! LGTM :)

hierarchicalforecast/utils.py Show resolved Hide resolved
@jmoralez jmoralez merged commit b841c2f into main Oct 3, 2023
@jmoralez jmoralez deleted the fix-aggregate branch October 3, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants