LinearModelParameters have two ways to get Weights, should be one

[Ivanidzo4ka]

we have this:

machinelearning/src/Microsoft.ML.StandardLearners/Standard/LinearModelParameters.cs

Line 93 in a0edc5c

public IReadOnlyList<float> Weights => new WeightsCollection(this);

and we have this:

machinelearning/src/Microsoft.ML.StandardLearners/Standard/LinearModelParameters.cs

Line 387 in a0edc5c

public void GetFeatureWeights(ref VBuffer<float> weights)

I would prefer to hide second one (and IHaveFeatureWeights interface) but it's up to discussion

[TomFinley]

I might actually prefer the opposite. It more closely resembles how IDataView and its schema and annotations are used. I also am unclear on the benefits of exposing ``IHaveFeatureWeights`, which is as an interface, AFAIK, not terribly well designed or thought out. (Does a multi-class logistic regression classifier have weights? How about OVA? How about an ensemble? How about a calibrated predictor.)

So I feel we must hide that interface. Whether or not we have one of these accessors or the other, I am slightly more indifferent. Either is fine I think. The latter is more in line with how we generally ask people to access data out of our core structures, so there is a consistency argument. The IReadOnlyList is easier to understand, and the performance reasons that lead us to prefer buffer reuse in IDataView itself are less important here.

[Ivanidzo4ka]

Sorry, I'm a bit confuse. I suggest to hide IHaveFeatureWeights and GetFeatureWeights method.
It looks like you prefer same thing, but you start your sentence with

I might actually prefer the opposite

[TomFinley]

Ah, I'm misspeaking I think.

So I would prefer to have the second one, that is, keep the ref, as it is consistent. But I would prefer to hide the interface itself. One can have an interface be internal while having its implementation remain public after all.

I don't insist on it though!! So I think we agree on hiding the interface, but might disagree on which of the two methods should be the actual accessors. But my disagreement on this part is not strong. There is a consistency argument w.r.t. IDataView itself for the ref, but we also expose model parameters as IReadOnlyList in many places since perf in these situations is always less critical.

So I agree with hiding the interface (I think I misunderstood you, and read "have" instead of "hide" on that point), and my disagreement on the other point is minor, and I'd be perfectly happy with your porposal being put into effect. I agree there should be only one in v1. (And if it becomes necessary for some reasson, we could always add the ref version later.)