MAINT: Rebase EBM utils

interpretml / interpret

Fit interpretable models. Explain blackbox machine learning.

https://interpret.ml/docs

MIT License

6.04k stars 714 forks source link

MAINT: Rebase EBM utils #517

Closed DerWeh closed 2 months ago

DerWeh commented 2 months ago

Simplify the utility functions in the EBM utils.

reduce code duplication
reduce complexity by early returns
explicitly unpack tuples of two elements
refactor methods
use dedicated functions of libraries

DerWeh commented 2 months ago

Thank you for the great package. Sadly, some of the high level code is written in a way I found quite complicated. I am currently trying to work through the code, trying to understand what's happening. If you are interested, I would offer to refactor some code on the way, as exemplified by this PR.

Please let me know in case you disagree about my suggested changes. Everybody has a different opinion what readable code is.

It might be necessary to extend the tests a little to make sure I didn't accidentally break any edge case, but the PR should not change any functionality. Though the module is sparsely documented, hence I am not certain if I correctly got the intent of the functions.

paulbkoch commented 2 months ago

Your changes seem generally good, and I agree they make the code easier to read. I'll take a closer look tomorrow. As you suspect, some of the utils (especially for merging EBMs) are less well tested than they should be.

paulbkoch commented 2 months ago

Everything other than the minor early return issue looks great! Thanks so much for simplifying this part of the code @DerWeh!!

DerWeh commented 2 months ago

@paulbkoch I added the special case with a minimal test. Good to merge from my side.

Edit: sorry, messed up committing the test..., now everything should be there.

paulbkoch commented 2 months ago

LGTM. Merging.

paulbkoch commented 2 months ago

And I fixed my bug that didn't handle scenarios where there were no cuts and only 1 interval.