-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fixed leaf_nodes computation #3645
Conversation
This should be ready for merge now. I don't know what's happening with coveralls |
Rebased on top of master to get coveralls parallel reports working. Hopefully, the coverage should be fixed now. |
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.
Only minor stylistic changes. It looks ready to go.
Thanks @rpgoldman for the careful review! I'll change what you suggested tomorrow |
You're very welcome! Thanks for doing this! It will make things much more testable, which is very good! |
Codecov Report
@@ Coverage Diff @@
## master #3645 +/- ##
=========================================
+ Coverage 89.8% 90.71% +0.9%
=========================================
Files 134 133 -1
Lines 20065 20386 +321
=========================================
+ Hits 18020 18493 +473
+ Misses 2045 1893 -152
|
@rpgoldman, I went through the suggestions you made and now this PR should be ready for review. |
It looks like the failing test was purely travis CI's fault |
Is this ready to merge @lucianopaz ? |
Noticed @junpenglao's comment and modified for numpy formatting.
@junpenglao, yes, it should be ready to merge. I can change the minor nitpick tomorrow if no one is in a rush to merge this |
Awesome! Great work as always ;-) |
This fixes #3643. I have to run now but I'll add a test, some release notes and refactor a bit of old dangling code by tomorrow. After that, it should be ok to merge.