interpretml / interpret

Fit interpretable models. Explain blackbox machine learning.
https://interpret.ml/docs
MIT License
6.22k stars 726 forks source link

add new reweight_terms method #467

Closed brandongreenwell-8451 closed 1 year ago

brandongreenwell-8451 commented 1 year ago

This PR addresses issue https://github.com/interpretml/interpret/issues/460. It adds a new method, called .reweight_terms(), to fitted EBM objects that effectively allows you to reweight (or entirely remove) individual terms (both main and interaction effects) from the model.

I'll follow up with another PR that add an example of editing the model by post-processing with the LASSO to introduce sparsity and reduce model complexity (e.g., going from 300+ terms to 20 or so in one example) while maintaining comparable accuracy. (You can see a brief example in https://github.com/interpretml/interpret/issues/460.)

brandongreenwell-8451 commented 1 year ago

Rebased with --signoff, so not sure why the DCO is still tripping?

paulbkoch commented 1 year ago

Hi @brandongreenwell-8451 -- Not sure on how to satisfy the DCO check, but I can merge it without it.

Once this model editing functionality is merged into the codebase, instead of writing some documentation to show how to use LASSO to reduce the number of terms and/or reweight them, I was wondering if it would make more sense to provide a "prune" member function that provides this service directly. The caller would pass in the number of terms they want to have (possibly also accepting 'auto'?), and the function would select which features to drop, and then call this more primitive function to perform the action.

In terms of the function in this PR, I believe callers might want to perform some of the actions it provides more individually. They might for example want to remove terms without reweighing other terms. They might want to set the intercept without reweighing or dropping terms, etc. My suggestion would be to break this single function up into 2 functions: remove_terms (which would take a list of term indexes or names to drop) and reweight (which would not handle the intercept or remove terms). For setting the intercept we can just make the caller handle that themselves since it's just one attribute they need to change. By default, if reweight is presented with zeros for any weights, I think it shouldn't eliminate those terms because that might surprise a caller who might not expect their term indexes to change within this call. We should just force the weights to zero. If a caller wants to do both, it isn't too costly to call reweigh first and incur the cost of zeroing the arrays followed by a call to remove_terms, which will change the indexes.

Another item I noticed: We can remove the check for multiclass and the exception "reweight_terms not supported for multiclass" since reweighing should work for multiclass.

brandongreenwell-8451 commented 1 year ago

Thanks for the comments @paulbkoch, makes sense to break it up into two methods. Here are my suggestions.

Option 1

For term removal, we'd have a method with the following signature (so there's no surprises to the caller, it seems logical to note in the docstrings which attributes are modified:

def remove_terms(self, term_list):
    """Removes terms (and their associated components) from a fitted EBM. Note 
    that this will change the structure (i.e., by removing the specified 
    indices) of the following components of ``self``: ``term_features_``,
    ``term_names_``, ``term_scores_``, ``bagged_scores_``, 
    ``standard_deviations_``, and ``bin_weights_``.

    Args:
        term_list: A list of term names or indices.

    Returns:
        Itself.
    """

And for reweighting terms, we'd have the following:

def reweight_terms(self, weights):
    """Reweight the individual term contributions. For example, you can 
    nullify the contribution of specific terms by setting their corresponding 
    weights to zero; this would cause the associated global explanations (e.g.,
    variable importance) to also be zero. A couple of things are worth noting: 
    1) this method has no affect on the fitted intercept and users will have to 
    change that attribute directly if desired, and 2) reweighting specific term 
    contributions will also reweight their related components in a similar 
    manner (e.g., variable importance scores, standard deviations, etc.).

    Args:
        weights: List of weights (one weight for each term in the model). 
            This should be a list or numpy vector (i.e., 1-d array) of 
            floats whose i-th element corresponds to the i-th element of the
            ``.term_*_`` attributes (e.g., ``.term_names_``).

    Returns: 
        Itself.
    """

Option 2

Leave reweight_terms() as is but change it to a standalone function that lives in _ebm/_research and have it return a modified deep copy of the original fit so as not to change anything in the original model.


I also really like the idea of a prune() type method that has the option to automatically determine which terms to drop and how to reweight, but I'd be concerned with the feasibility of handling this given the limitations of sklearn's support for L1 regularization to general link functions. Putting this on the user gives them the flexibility to to use a variety of techniques, like E-net with an additional L2 penalty, etc.

Thoughts? If you're on board for Option 1, I can break it up into two methods and update the PR.

brandongreenwell-8451 commented 1 year ago

I'm curious on the best way to use these two methods to accomplish the previous example using the LASSO. For instance, let's say I used the ISLE approach to compute weights for each term contribution (as in the original example), so I modify the fit by calling reweight_terms() and passing in the associated coefficients; many of which could be zero (then handle the intercept manually). If I wanted to reduce the model size accordingly, I'd call remove_terms() but as a user in this regard I'd have to figure out the term IDs to remove (ideally by identifying the IDs of the zero coefficients). Do you think this is reasonable? Would it be useful to have an additional option, like zero_contribution=False such that when set to True (and term_list=None) then it would just remove all terms that have zero contribution to the fit/predictions?

paulbkoch commented 1 year ago

In python getting the list/array to pass to remove_terms is a one-liner:

#if weights is a numpy array (this gets a bool array)
terms = weights == 0

# if weights can be either a list or numpy array
terms = [index for index, value in enumerate(weights) if value == 0]

But, having said that, I do like the idea of having a zero_contribution parameter since it would be a common pattern. I might instead name it something like remove_zeros since the potentially surprising action is then part of the parameter name. Alternatively, we might want to pass in a threshold? Something like trim=0.1 would remove the terms for anything with a weight of 0.1 and lower? Not sure how useful that would be in practice though.

paulbkoch commented 1 year ago

You raise some good points given there are multiple ways to achieve pruning. There's probably a benefit in providing a built-in way to do it though since most package users would probably prefer to have something that does roughly what they want rather than spend the time to think about (and implement) the best way to do it. And people who really care can still handle it whatever way they want if we also give them the more primitive functionality to handle reweighing.

It's not ideal, but perhaps for now we could throw an exception if the EBM uses a non-default objective? And if that tweaks somebody they can put in a future PR either to us or in scikit-learn to add more options to the LASSO implementation.

brandongreenwell-8451 commented 1 year ago

@paulbkoch would it be reasonable to start with the reweight and remove function described above, then follow up with another PR down the road?

paulbkoch commented 1 year ago

Sounds good @brandongreenwell-8451. These functions are useful by themselves.

brandongreenwell-8451 commented 1 year ago

@paulbkoch PR updated with the changes discussed above. Feel free to modify any of the docstrings, etc. E.g., I used remove_nil_terms as the optional argument in remove_terms(), but it could easily be named something else you feel is more meaningful. Once this goes through I may end up arXiving a short paper on how to introduce sparsity in EBMs via the LASSO that maybe the docs can point to at a later date.

paulbkoch commented 1 year ago

Hi @brandongreenwell-8451 -- An ArXiv paper would be great.

On the PR, I just realized that I misunderstood your original thinking. My thought was that there could be two functions with the following signatures:

def remove_terms(self, terms) and def reweight_terms(self, weights, remove_nil_terms=False)

Then the ISLE implementation would probably just call reweight_terms with remove_nil_terms=True and it wouldn't call remove_terms directly. reweight_terms could call remove_terms internally though to remove any terms with a weight of 0 if remove_nil_terms was true.

Thoughts?

brandongreenwell-8451 commented 1 year ago

Hi @brandongreenwell-8451 -- An ArXiv paper would be great.

On the PR, I just realized that I misunderstood your original thinking. My thought was that there could be two functions with the following signatures:

def remove_terms(self, terms) and def reweight_terms(self, weights, remove_nil_terms=False)

Then the ISLE implementation would probably just call reweight_terms with remove_nil_terms=True and it wouldn't call remove_terms directly. reweight_terms could call remove_terms internally though to remove any terms with a weight of 0 if remove_nil_terms was true.

Thoughts?

Apologies, and that actually a bit more sense now that I think about it. I'll make the adjustment and update shortly. For the latter, I think it makes most sense to just have remove_terms only remove terms specified by the user, but it could always be added in later to that method too if others think it would be useful! Sound good?

paulbkoch commented 1 year ago

Hi @brandongreenwell-8451 -- We definitely need some kind of remove_terms function that only operates on the terms given to it by the caller. The eventual ISLE function does not need this, but EBMs do as a general ability, so I figure it's best to have it as a primitive that is called by reweight.

Another small nuance: I think the reweigh_terms function should only remove terms that have 0 in the given weight parameter, not check if any of the resulting term scores are all zero. If some of the terms had all zeros in their scores before the call to reweigh_terms, then the caller would need to do a lot of checks to know which terms to expect will be dropped, and that behavior would be surprising, I think. Safer to force the caller to be more explicit in this case and call remove_terms themselves in the special case that there are terms with all zeros and they want them removed. It's pretty unlikely we'd see a term with all zeros naturally anyways, except if the user previously edited the EBM (in which case whatever they called to do that could also have a remove_nil_terms parameter), or in the case of a feature with only 1 value. In the latter case LASSO would give it a weight of 0 anyways I think, so those terms would be dropped anyways in an ISLE implementation.