Better names to calibreated linear classification models#3034
Better names to calibreated linear classification models#3034wschin merged 17 commits intodotnet:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3034 +/- ##
=========================================
Coverage ? 72.52%
=========================================
Files ? 804
Lines ? 144157
Branches ? 16178
=========================================
Hits ? 104552
Misses ? 35193
Partials ? 4412
|
| /// Binary Classification trainer estimators. | ||
| /// </summary> | ||
| public static class LbfgsBinaryClassificationStaticExtensions | ||
| public static class LbfgsBinaryExtensions |
There was a problem hiding this comment.
LbfgsBinaryExtensions [](start = 24, length = 21)
I think these are just static extensions, if possible could you keep static in the name of the class? #Resolved
| /// <include file='doc.xml' path='doc/members/member[@name="LBFGS"]/*' /> | ||
| /// <include file='doc.xml' path='docs/members/example[@name="LogisticRegressionBinaryClassifier"]/*' /> | ||
| public sealed partial class LogisticRegressionBinaryTrainer : LbfgsTrainerBase<LogisticRegressionBinaryTrainer.Options, | ||
| public sealed partial class LbfgsLogisticRegressionTrainer : LbfgsTrainerBase<LbfgsLogisticRegressionTrainer.Options, |
There was a problem hiding this comment.
LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
There was a problem hiding this comment.
It looks fine if we consider this trainer and its SDCA counterpart (they all solve LR). However, it will break the consistency between LBFGS trainers. Please take a look at Iteration 3.
LBFGS trainer names:
LbfgsLogisticRegressionTrainer
LbfgsPoissonRegressionTrainer
LbfgsMaximumEntropyTrainer
Note that we can't drop model names (such as LogisticRegression) because we need PoissonRegression to warn users that PoissonRegression is not a common case.
In reply to: 267460936 [](ancestors = 267460936)
There was a problem hiding this comment.
There was a problem hiding this comment.
yeah, given the target user base, IMO should do a one off for this one, and keep binary in the name.
(also this feels like a classic academic vs engineering solution of a problem :D )
Also, IMO not all details need to be in the name. IMO we don't need to distingush that this is Lbfgs logistic regression through renaming. It can be in the documentations. shorter names are better...
In reply to: 267477762 [](ancestors = 267477762,267475700,267460936)
There was a problem hiding this comment.
It will make the API looks like mlContext.BinaryClassification.LbfgsBinary(...) so I switch to SDCA-style name LbfgsCalibrated.
In reply to: 267481155 [](ancestors = 267481155,267477762,267475700,267460936)
There was a problem hiding this comment.
yeah, given the target user base, IMO should do a one off for this one, and keep binary in the name.
(also this feels like a classic academic vs engineering solution of a problem :D )
Also, IMO not all details need to be in the name. IMO we don't need to distingush that this is Lbfgs logistic regression through renaming. It can be in the documentations. shorter names are better...
There are several trainers returning the same model types. Only this one is called LogisticRegression and this one has been outperformed by other algorithms. Thus, I want to append Lbfgs to emphasize that this is not the only trainer for logistic regression.
In reply to: 267477762 [](ancestors = 267477762,267475700,267460936)
#Resolved
| /// </format> | ||
| /// </example> | ||
| public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog, | ||
| public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog, |
There was a problem hiding this comment.
LbfgsLogisticRegressio [](start = 53, length = 22)
you miss n in the end.
And I don't think this new verb any better than previous one.
More importantly I don't understand why you even changing this? We had @agoswami issue and PR regarding names, where we spend quite a lot of effort in attempt to figure out names, why you change it out of blue now? #Resolved
There was a problem hiding this comment.
| .Append(ml.Transforms.Concatenate("Features", "TextFeatures", "age", "fnlwgt", | ||
| "education-num", "capital-gain", "capital-loss", "hours-per-week")) | ||
| .Append(ml.BinaryClassification.Trainers.LogisticRegression()); | ||
| .Append(ml.BinaryClassification.Trainers.LbfgsCalibrated()); |
There was a problem hiding this comment.
LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
There was a problem hiding this comment.
Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
There was a problem hiding this comment.
As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
There was a problem hiding this comment.
Technically true, but wouldn't it be clearer from a user perspective to call it LogisticRegression? Everybody knows what that is. Behind the scenes, many packages solve LR with variants of L-BFGS (e.g. Spark), so I don't think it would be misleading to say that it's called LogisticRegression. We can put technical details like "Implemented with L-BFGS etc." in the docs.
Look at FastTree. We don't call that CalibratedLambdaMART ;)
In reply to: 267857743 [](ancestors = 267857743)
There was a problem hiding this comment.
A problem here is that L-BFGS is not good for large-scale sparse data sets. SDCA in that case is usually much faster. If we call LBFGS-LogisticRegression just LogisticRegression, external users may view LBFGS as the default solver to all logistic regression problems. #Resolved
| .Append(ml.Transforms.Concatenate("Features", "TextFeatures", "age", "fnlwgt", | ||
| "education-num", "capital-gain", "capital-loss", "hours-per-week")) | ||
| .Append(ml.BinaryClassification.Trainers.LogisticRegression()); | ||
| .Append(ml.BinaryClassification.Trainers.LbfgsLogisticRegression()); |
There was a problem hiding this comment.
How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
There was a problem hiding this comment.
We can always add that at a later time. #Resolved
|
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
Why do we think adding prefix "Lbfgs" in the name is a better alternative than what we have currently ? In fact to me it seems having 'Lbfgs' as prefix might throw users off
|
Ivanidzo4ka
left a comment
There was a problem hiding this comment.
Let me block it for now.
I don't think we have agreement among all of us regarding how positive this change is.
I believe @wschin answered this in the top comment to the PR:
It seems to me that is his exact intention here. Don't use this one when you want to do LR. Or at least, don't think this is the "good"/"default" one. We have a better one. #Resolved |
|
I think that it would also make sense to convert the binary |
|
LBFGS can mean LBFGS family algorithms.
It's a variant of LBFGS, as stated in your link.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS. #Resolved |
|
Couple of notes after discussing with @wschin
|
Remove empty line to trigger build
1a8d2da to
8af0d21
Compare
| using Microsoft.ML.Transforms; | ||
|
|
||
| [assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options), | ||
| [assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options), |
There was a problem hiding this comment.
SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
There was a problem hiding this comment.
@rogancarr..could you cross-check ? To me the Class names do seem to match up fine, with both having the word Binary in the name
LbfgsLogisticRegressionBinaryTrainerSymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
There was a problem hiding this comment.
Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
There was a problem hiding this comment.
Got it. I was mixing up catalogs & classes. #Resolved
|
|
||
| /// <summary> | ||
| /// Predict a target using a linear classification model trained with <see cref="SdcaCalibratedBinaryTrainer"/>. | ||
| /// Predict a target using a linear classification model trained with <see cref="SdcaLogisticRegressionBinaryTrainer"/>. |
There was a problem hiding this comment.
SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
There was a problem hiding this comment.
Yes, adding both of LogisticRegression and Binary to trainer classes was our decision. For API, we only append LogisticRegression because Binary naturally comes from their context.
In reply to: 268859905 [](ancestors = 268859905)
Fix #3016 by renaming
LogisticRegressionBinaryTrainertoLbfgsLogisticRegressionTrainer. Note that for multiclass case, we haveLbfgsMaximumEntropyTrainer. In addition, as SDCA (aka dual coordinate descent methods) outperforms L-BFGS when training logistic regression models, we should not make L-BFGS looks like the default trainer of logistic regression model.This link contains our the conclusion of those new names.