Skip to content

OLS FeatureWeights are not the model weights #1850

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

Closed
rogancarr opened this issue Dec 7, 2018 · 11 comments · Fixed by #3339
Closed

OLS FeatureWeights are not the model weights #1850

rogancarr opened this issue Dec 7, 2018 · 11 comments · Fixed by #3339
Assignees
Labels
bug Something isn't working question Further information is requested usability Smoothing user interaction or experience

Comments

@rogancarr
Copy link
Contributor

For the LinearPredictor, the GetFeatureWeights returns the model weights, as expected:

public virtual void GetFeatureWeights(ref VBuffer<Float> weights)
{
    Weight.CopyTo(ref weights);
}

The OlsLinearRegressionPredictor has the same method, but it does not return the model weights. It returns the -log(p-value) for each feature. This is weird because it overrides the LinearRegressionPredictor's GetFeatureWeights method but returns a different kind of value, essentially the magnitudes of the p-values.

Now, this goes back to the meaning of feature weight. Many predictors that are not linear models implement GetFeatureWeight, and it looks like it's a measure of importance. What's the right thing here? Do we want to return a measure of relative importance or do we want to return the model weights?

Currently, I find this API to be super confusing because there is no docstring to explain what you are getting back, and I would expect the "Feature Weights" of a linear model to the model weight parameters.

Also, this is a nit, but if we really want to return the magnitude of the p-values, doesn't -log10(p-value) make more sense? The base e log puts these onto a weird scale.

@rogancarr
Copy link
Contributor Author

@yaeldekel @glebuk Are you all familiar with these APIs and what they are supposed to mean?

@rogancarr
Copy link
Contributor Author

Also, to be clear, the p-value for a feature doesn't specify the relative importance of the feature; it just means how confidence one can be that the value is different than zero. You can have very unimportant features that you are very confident are not zero....

@rogancarr rogancarr added question Further information is requested usability Smoothing user interaction or experience labels Dec 7, 2018
@rogancarr
Copy link
Contributor Author

@najeeb-kazmi You just touched up the linear model public APIs -- do you have any thoughts on this?

@TomFinley
Copy link
Contributor

Now, this goes back to the meaning of feature weight. Many predictors that are not linear models implement GetFeatureWeight, and it looks like it's a measure of importance. What's the right thing here? Do we want to return a measure of relative importance or do we want to return the model weights?

Hi @rogancarr , it has been many years since I wrote this code, but the request for "feature importance" remains the motivation for the differentiation here. In something like a stochastic method for learning a linear predictor, it is true that the coefficients themselves might be the best (though deeply imperfect) means of measuring feature importance. However a statistical method (like OLS) we can actually do a bit better here. The goal as you've observed is to get the model's best opinion of what it considers its per-feature importance measures, as opposed to, say, "coefficients if it's a linear model and something else if not."

The issue here might be one of naming -- it is perhaps wrong to use the word "weight" since that is also used in the case of linear models specifically to mean "coefficient." Would renaming potentially help?

@rogancarr
Copy link
Contributor Author

rogancarr commented Dec 13, 2018

Hi @TomFinley ,

Thanks for the insight here! If the meaning is around feature importance, then I'd say we rename the API to reflect that. The word weight is overloaded, unfortunately. In my practice, I was actually using this method to get out feature weights from the usual LinearPredictor models. It was only when I swapped in an OlsLinearPredictor class that I noticed that I was getting strange results. Having a specifically named API would be useful.

Now, there is also a second point here. Since it's a measure of importance, then the way we calculate it using a p-value isn't really useful. The p-value simply tells us the confidence that we can have that the coefficient should not be zero; it doesn't actually tell us how important the coefficient is to the model, nor does it indicate the relative sizes of the coefficient (as the standard deviations are different for each coefficient). However, now that I've criticized our current importance measure, I'm not sure what to recommend instead: I'm just not sure there is a great way to specify feature importance other than by the coefficients themselves. Usually we have to model or measure importance, as we do in the Feature Contribution Calculator, Permutation Feature Importance, or related methods.

Edit: Note that I'm talking about coefficients as a proxy for importance when the data is standardized.

@shauheen shauheen added the bug Something isn't working label Jan 8, 2019
@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Feb 9, 2019

Since I'm touching HAL assembly, I get to the point you guys discussing, and I want to make changes.
But I want to make sure I understand you both right.
So what I'm plan to do right now is to:

  • Feature weights for OLS will return original weights for linear model.

  • OLS will have GetCoefficents method which will return modified PValues as coefficents.

Does that sound fair? Or I didn't get you right?
Or you want to have GetCoefficents across all linear predictors and use them in feature importance?

Also shall we make GetFeatureWeights internal since it operates over VBuffer we already have Weights property?

@rogancarr
Copy link
Contributor Author

rogancarr commented Feb 9, 2019

hi @Ivanidzo4ka !

FeatureWeights() as linear model weights makes sense to me.

GetCoefficients should probably also return the model weights? I don't think we should return the modified p-values.

FeatureWeights is some sort of measure of feature importance. I kinda want to make it internal, but I think it's the only way to get feature importance for Trees currently. Let's discuss this before we make a move.

@Ivanidzo4ka
Copy link
Contributor

I think it shouldn't be hard to make GetFeaturesWeights as internal method. But it probably belongs to cleaning predictor interface and not this exact issue.
GetFeatureWeights for OLS would return weights of model, and if user want to have coefficients he can always access PValue which are public, and use them.

@rogancarr
Copy link
Contributor Author

Moving GetFeaturesWeights internal may make it hard to get feature importance from Trees. Let me look into this on Monday.

@rogancarr
Copy link
Contributor Author

rogancarr commented Feb 15, 2019

@Ivanidzo4ka I looked into this a bit:

GetFeatureWeights() is only used in linear models (discussed here) and in Tree models to return Feature Gains. There is an interface IHaveFeatureWeights and IPredictorWithFeatureWeights. From what I can tell, these are only really used in PermutationFeatureImportance.

I'd like to mark this method Internal and expose an explicit GetFeatureGains on our tree models — honestly, the name GetFeatureWeights isn't clear at all, and isn't documented anywhere, so you really have to read the source to know that it computes feature gains.

What do you think about that?

@Ivanidzo4ka and @TomFinley?

@rogancarr
Copy link
Contributor Author

Update to the current status:

The tree model parameters still have this interface and it is used to get the feature gain. This is fine, except that nowhere does it say that this is feature gain. I suggest we add documentation to this function in the tree classes that says that it's feature gain and then close this issue.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working question Further information is requested usability Smoothing user interaction or experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants