Skip to content

Fix division error in inactive leaf size estimation#1721

Merged
kulbachcedric merged 6 commits intoonline-ml:mainfrom
kulbachcedric:Fix-ht-division-error
Nov 13, 2025
Merged

Fix division error in inactive leaf size estimation#1721
kulbachcedric merged 6 commits intoonline-ml:mainfrom
kulbachcedric:Fix-ht-division-error

Conversation

@kulbachcedric
Copy link
Contributor

Hi all,
just added the check for devision errors as mentioned in #1717

Best
Cedric

@e10e3
Copy link
Contributor

e10e3 commented Nov 10, 2025

Hello Cedric,

Are you sure this is the correct fix?
The issue mentioned a problem with the number of active leaves (the line above).

@kulbachcedric
Copy link
Contributor Author

Hi @e10e3
you're right, but are we not eventually facing the same issue in line 301?

Copy link
Member

@smastelini smastelini left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @kulbachcedric to look into that! Would you mind adding an entry in the release notes?

@smastelini
Copy link
Member

Sorry! I didn't see the use of the active leaves in the lines above.

@e10e3
Copy link
Contributor

e10e3 commented Nov 11, 2025

are we not eventually facing the same issue in line 301?

Yes, we are, you're correct.
The code is similar, and the same problem could happen with the next division, so both line 299 and line 301 can benefit from the same fix you propose.

Your first commit was not wrong per se, but because you mentioned that you just applied the suggestions from the issue reporter, I wanted to point out that the issue was about a different line, so that you didn't end up patching a different problem while keeping the original one unsolved 😁.

In the end, both lines are affected by the same bug and a complete fix would apply to both.

@e10e3
Copy link
Contributor

e10e3 commented Nov 13, 2025

Thank you for adding a changelog note. Don't you think it should go in another section? Previous changelogs had a section for each module, so it would make sense to sort this change under “tree” rather than “stats”.

Co-authored-by: Émile <73942755+e10e3@users.noreply.github.com>
@kulbachcedric kulbachcedric merged commit a16d0f1 into online-ml:main Nov 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants