-
Notifications
You must be signed in to change notification settings - Fork 77
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
Reduce number of trees in RF regressors #1294
Conversation
Reduced to 150 to 50 trees, physics performance is not impacted, while memory needs will decrease (and speed increase)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to upload the comparison in the wiki for bookkeeping
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1294 +/- ##
==========================================
+ Coverage 73.49% 73.50% +0.01%
==========================================
Files 134 134
Lines 14210 14210
==========================================
+ Hits 10443 10445 +2
+ Misses 3767 3765 -2 ☔ View full report in Codecov by Sentry. |
Some comparisons here: RF_tree_reduction.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not changing the classifiers parameters as well?
The tests for that (since they involve background, ideally from real data) are more complicated. |
there is something wrong with mamba setup step in the CI (is taking > 50 min) |
Can we ignore it? |
Tests will not run either until we merge #4563 or the problem with mamba is solved. Anyway, I think the changes here are not tested. I'd say it can be merged |
Reduced to 150 to 50 trees, physics performance is not impacted, while memory needs will decrease (and speed increase)