Public API for KMeansPredictor #1739
Conversation
…d internalize interfaces except ICanSaveModel
| @@ -169,7 +172,7 @@ private void Map(in VBuffer<Float> src, Span<Float> distances) | |||
| } | |||
| } | |||
|
|
|||
| public void SaveAsText(TextWriter writer, RoleMappedSchema schema) | |||
| void ICanSaveInTextFormat.SaveAsText(TextWriter writer, RoleMappedSchema schema) | |||
| { | ||
| /// <include file='./doc.xml' path='doc/members/member[@name="KMeans++"]/*' /> | ||
| public class KMeansPlusPlusTrainer : TrainerEstimatorBase<ClusteringPredictionTransformer<KMeansPredictor>, KMeansPredictor> | ||
| public class KMeansPlusPlusTrainer : TrainerEstimatorBase<ClusteringPredictionTransformer<KMeansModelParameters>, KMeansModelParameters> |
There was a problem hiding this comment.
Can Visual Studio retrieve the doc in xml? If not, we need to add enough information here.
najeeb-kazmi
requested review from
sfilipi,
TomFinley,
Zruty0 and
Ivanidzo4ka
najeeb-kazmi
changed the title
| @@ -185,7 +185,8 @@ protected LinearPredictor(IHostEnvironment env, string name, ModelLoadContext ct | |||
| _weightsDenseLock = new object(); | |||
| } | |||
|
|
|||
| protected override void SaveCore(ModelSaveContext ctx) | |||
| [BestFriend] | |||
| private protected override void SaveCore(ModelSaveContext ctx) | |||
There was a problem hiding this comment.
private [](start = 8, length = 7)
This is already a protected function on an internal class -- isn't that already hidden?
There was a problem hiding this comment.
Sorry I misread - it is a public class and you are overriding SaveCore from PredictorBase which is protected. However, I dont quite follow how protected isnt hidden to begin with -- is this covering cases where classes derive from LinearPredictor? Also could LinearPredictor be internal?
In reply to: 236912196 [](ancestors = 236912196)
There was a problem hiding this comment.
Hi @singlis , sorry just saw this now -- protected is absolutely not hidden. It won't show up in usage of instances, but will show up in docs and other contexts.
Hi @najeeb-kazmi, thanks for doing this! The corresponding interface has been made internal, should we make these explicit implementations here and everywhere? It requires a little bit of work, but no more than you've already done for the Refers to: src/Microsoft.ML.FastTree/FastTree.cs:2968 in 073ff03. [](commit_id = 073ff03, deletion_comment = False) |
| @@ -296,7 +296,7 @@ private OlsLinearRegressionPredictor TrainCore(IChannel ch, FloatLabelCursor.Fac | |||
| Double tss = 0; // total sum of squares | |||
| using (var cursor = cursorFactory.Create()) | |||
| { | |||
| var lrPredictor = new LinearRegressionPredictor(Host, in weights, bias); | |||
| IValueMapper lrPredictor = new LinearRegressionPredictor(Host, in weights, bias); | |||
There was a problem hiding this comment.
IValueMapper lrPredictor [](start = 16, length = 24)
Good stuff
There was a problem hiding this comment.
Thank you for dogin this @najeeb-kazmi !! This will lay the groundwork for lots more hiding in future.
|
|
||
| namespace Microsoft.ML.Trainers.KMeans | ||
| namespace Microsoft.ML.KMeansClustering |
There was a problem hiding this comment.
Microsoft.ML.KMeansClustering [](start = 10, length = 29)
i wonder if we want to put all predictors in something:
Microsoft.ML.ModelParameters.KMeansClustering
There was a problem hiding this comment.
If the argument is that the namespace is "Trainers" and therefore only ITrainer implementors should live in that namespace, then I'd argue that we should not have "Trainers" in the namespace name, if you think it could cause confusion.
It would be very inconvenient to have predictors in a different namespace from their associated trainers (except in generalizable cases like linear predictors that can be produced by many things).
In reply to: 238843887 [](ancestors = 238843887)
There was a problem hiding this comment.
Maybe Microsoft.ML.KMeans would be a better namespace, but let's not have one namespace every time we devise a new type that isn't a trainer, a predictor, or whatever. 😛
There was a problem hiding this comment.
On #1318 there was an explicit ask to put all the trainers inside Microsoft.ML.Trainers or appropriate namaspaces.
So if i read both those issues, the ModelParameters also go in the same namespace as the respective trainer; for this case Microsoft.ML.Trainers.Kmeans @TomFinley that sounds good?
There was a problem hiding this comment.
That makes sense. I'll stick to the conventions in #1318 and revert the namespace to Microsoft.ML.Trainers.KMeans.
ghost
locked as resolved and limited conversation to collaborators
Fixes #1699