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.

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

Create offset.py #371

Closed
wants to merge 1 commit into from
Closed

Conversation

spyrosUofA
Copy link

Adding offset to EBM Classifier and Regressor (and other dependencies)

Signed-off-by: Spyros [email protected]

Adding offset to EBM Classifier and Regressor (and other dependencies) 

Signed-off-by: Spyros <[email protected]>
@paulbkoch
Copy link
Collaborator

Hi @spyrosUofA -- Thanks for this PR! I had a look over it and have a few comments:

I see the automated tests are failing on this line where offset.py is attempting to reference the EBMPreprocessor class https://github.com/spyrosUofA/interpret/blob/a81310bd21c8fc23efee28a4cede44e0780a82c8/python/interpret-core/interpret/glassbox/ebm/offset.py#L45

The EBMPreprocessor class had been moved to bin.py at this point in development:
https://github.com/spyrosUofA/interpret/blob/a81310bd21c8fc23efee28a4cede44e0780a82c8/python/interpret-core/interpret/glassbox/ebm/bin.py#L1509

And in the most recent code, it has been moved again to this location:

class EBMPreprocessor(BaseEstimator, TransformerMixin):

If my understanding is correct the "Off" parameter functions similarly to the "init_score" parameter in LightGBM ( https://lightgbm.readthedocs.io/en/latest/pythonapi/lightgbm.LGBMClassifier.html#lightgbm.LGBMClassifier ). If that is correct, I would recommend using the LightGBM naming in order to make it easier to share calling code between the two and potentially leverage existing user knowledge of their parameters.

At a higher level, the current PR replicates a lot of code that exists in other locations within the codebase. I understand why this was done since a lot has changed in the preprocessing section. It would be better in terms of maintainability though to try and re-use as much of the other code as possible by passing init_score through the existing utility functions. In the same vein I think init_score could, similarity to LightGBM, be a parameter to the fit function for all classes that derive from the EBMModel class instead of creating the new OffsetExplainableBoostingRegressor and OffsetExplainableBoostingClassifier classes.

One section of our code that I recommend reading over is the interaction detection code. We have a similar requirement to use existing scores there, and do very similar processing:

@spyrosUofA
Copy link
Author

Hi @paulbkoch, thank you for your reply. Indeed, this offset code was built for v0.27. @tony-mtl was able to add the offset to v0.30 and addressed the above comments. Committing the new code now!

@paulbkoch paulbkoch mentioned this pull request Feb 16, 2023
@paulbkoch
Copy link
Collaborator

Closing this PR since your newer PR that adds init_score #426 has been merged.

@paulbkoch paulbkoch closed this May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants