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

fix: handle zero division in top down methods #325

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

fix: handle zero division in top down methods #325

wants to merge 5 commits into from

Conversation

mattbuot
Copy link

This is to address #324.

First contribution to this repo, please let me know if any other change is required, I tested for all the reconciliation methods that use the top down logic: TopDown, TopDownSparse, MiddleOut, MiddleOutSparse.

Not sure why some formatting was applied in some other files when I built the library... The only real change is in hierarchicalforecast/methods.py

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@CLAassistant
Copy link

CLAassistant commented Jan 30, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@elephaint
Copy link
Contributor

elephaint commented Jan 30, 2025 •
edited
Loading

@mattbuot Thanks for the contribution!

I see some of your contributions have been made in a .py file directly - is that the error you are referring to? Maybe you didn't set up your environment correctly with the pre-commit hooks and nbdev hooks?

I took the liberty to fix the linting, and make a small change to properly deal with floating numbers. PR is good to go imho.

@elephaint elephaint linked an issue Jan 31, 2025 that may be closed by this pull request
TopDown reconciliation produces NaNs when children sum to 0 #324
Open
elephaint
elephaint approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@elephaint elephaint 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 your contribution!

@elephaint
Copy link
Contributor

@mattbuot Can you make sure the user Matthieu signs the CLA too? I'm assuming it's your account too?

@mattbuot mattbuot force-pushed the fix_top_down_zero_div branch from 3337708 to 5ec9b3d Compare January 31, 2025 23:04
@mattbuot
Copy link
Author

@elephaint thanks for cleaning up the PR, I removed the other Matthieu from the authors. Somehow I had made these commits without my github email.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elephaint elephaint elephaint 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.

TopDown reconciliation produces NaNs when children sum to 0
3 participants
@mattbuot @CLAassistant @elephaint