Better names to calibreated linear classification models - #3034

Merged
wschin merged 17 commits into
dotnet:masterfrom
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged

Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom
wschin:sync-lbfgs-lr-me

Conversation

@wschin

@wschinwschin commented Mar 20, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.

@wschinwschin added the API Issues pertaining the friendly API label Mar 20, 2019
@wschinwschin self-assigned this Mar 20, 2019
@codecov

codecovBot commented Mar 20, 2019•
edited
Loading

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.
The diff coverage is 96.22%.

@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more

artidoro
artidoro reviewed Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs Outdated
/// Binary Classification trainer estimators.
/// </summary>
public static class LbfgsBinaryClassificationStaticExtensions
public static class LbfgsBinaryExtensions

@artidoroartidoroMar 20, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

artidoro
artidoro approved these changes Mar 20, 2019
View reviewed changes

@artidoroartidoro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@yaeldekel

yaeldekel commented Mar 20, 2019•
edited by wschin
Loading

Copy link
Copy Markdown 
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix

Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)

@wschin

wschin commented Mar 20, 2019

Copy link
Copy Markdown
ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.

In reply to: 474926238 [](ancestors = 474926238)

Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)

Ivanidzo4ka
Ivanidzo4ka reviewed Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs Outdated
/// <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,

@Ivanidzo4kaIvanidzo4kaMar 20, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)

Can you call it LbfgsBinaryTrainer? #Resolved

@wschinwschinMar 20, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.
Putting word Regression without Binary in the trainer is confusing.
If I would saw name of this trainer I would assume it's part of regression task.
Which is not.

In reply to: 267475700 [](ancestors = 267475700,267460936)

@sfilipisfilipiMar 20, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@wschinwschinMar 20, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@wschinwschinMar 21, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ivanidzo4ka
Ivanidzo4ka reviewed Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs Outdated
/// </format>
/// </example>
public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,

@Ivanidzo4kaIvanidzo4kaMar 20, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wschinwschinMar 20, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.

In reply to: 267478532 [](ancestors = 267478532)

@rogancarrrogancarrMar 20, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.

In reply to: 267481247 [](ancestors = 267481247,267478532)

rogancarr
rogancarr reviewed Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs Outdated
.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());

@rogancarrrogancarrMar 20, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)

I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved

@rogancarrrogancarrMar 20, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved

@wschinwschinMar 21, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved

@rogancarrrogancarrMar 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@wschinwschinMar 22, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rogancarr
rogancarr reviewed Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
.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());

@rogancarrrogancarrMar 22, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved

@rogancarrrogancarrMar 22, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved

rogancarr
rogancarr previously approved these changes Mar 22, 2019
View reviewed changes

@rogancarrrogancarr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@wschin
wschin requested a review from abgoswamMarch 22, 2019 20:57
@abgoswam

abgoswam commented Mar 22, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters

This PR proposes to modify the existing APIs as follows :

  • Renames several MLContext names
    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names
    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer

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

  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .

  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.

@eerhardt@sfilipi #Resolved

@Ivanidzo4ka
Ivanidzo4ka self-requested a review March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka previously requested changes Mar 22, 2019
View reviewed changes

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.
I don't think we have agreement among all of us regarding how positive this change is.

@eerhardt

eerhardt commented Mar 22, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Member

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

I believe @wschin answered this in the top comment to the PR:

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.

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

@rogancarr

rogancarr commented Mar 22, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved

@abgoswam

abgoswam commented Mar 22, 2019•
edited
Loading

Copy link
Copy Markdown
Member

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.

  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself

  • Scikit-learn solves this by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.
Algorithm to use in the optimization problem. #Resolved

@wschin

wschin commented Mar 23, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.

* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.

* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.
Algorithm to use in the optimization problem.

No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.

#Resolved

@abgoswam

abgoswam commented Mar 25, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Couple of notes after discussing with @wschin

  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.

  • We can take inspiration from Spark.ML to finilize the API

  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved

@Ivanidzo4ka
Ivanidzo4ka dismissed their stale reviewMarch 25, 2019 21:06

revoking review

abgoswam
abgoswam approved these changes Mar 25, 2019
View reviewed changes

@abgoswamabgoswam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin changed the title Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr dismissed their stale reviewMarch 25, 2019 21:25

revoking review

rogancarr
rogancarr reviewed Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),

@rogancarrrogancarrMar 25, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)

LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved

@abgoswamabgoswamMar 25, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer

In reply to: 268859710 [](ancestors = 268859710)

@wschinwschinMar 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.

In reply to: 268865148 [](ancestors = 268865148,268859710)

@rogancarrrogancarrMar 25, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved

rogancarr
rogancarr reviewed Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

/// <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"/>.

@rogancarrrogancarrMar 25, 2019•
edited by wschin
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)

Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved

@wschinwschinMar 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@wschin
wschin merged commit 8730c87 into dotnet:masterMar 25, 2019
@wschin
wschin deleted the sync-lbfgs-lr-me branch March 25, 2019 23:47
@zeahmedzeahmed mentioned this pull request Mar 29, 2019
Merged
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims
4 more reviewers
@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Better names to calibreated linear classification models - #3034
Merged
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Mar 25, 2019
Merged
Better names to calibreated linear classification models#3034
wschin merged 17 commits into
dotnet:masterfrom 
wschin:sync-lbfgs-lr-me
Conversation
@wschin
@wschinwschin
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fix#3016 by renaming LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer. Note that for multiclass case, we have LbfgsMaximumEntropyTrainer. 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.
@wschinwschin
added
the 
API
Issues pertaining the friendly API
label
Mar 20, 2019
@wschinwschin
self-assigned this
Mar 20, 2019
@codecov
codecovBot
commented
Mar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@5f9be36). Click here to learn what that means.

The diff coverage is 96.22%.
@@ Coverage Diff @@## master #3034 +/- ##
=========================================
Coverage ? 72.52% =========================================
Files ? 804 Lines ? 144157 Branches ? 16178 =========================================
Hits ? 104552 Misses ? 35193 Partials ? 4412
FlagCoverage Δ
#Debug72.52% <96.22%> (?)
#production68.16% <80%> (?)
#test88.72% <100%> (?)
Impacted FilesCoverage Δ
...LogisticRegression/MulticlassLogisticRegression.cs65.87% <ø> (ø)
test/Microsoft.ML.Functional.Tests/Training.cs100% <100%> (ø)
...est/Microsoft.ML.StaticPipelineTesting/Training.cs99.28% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs98.02% <100%> (ø)
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs63.8% <100%> (ø)
test/Microsoft.ML.Tests/Scenarios/OvaTest.cs100% <100%> (ø)
...soft.ML.Tests/PermutationFeatureImportanceTests.cs100% <100%> (ø)
.../Standard/LogisticRegression/LogisticRegression.cs94.95% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Evaluation.cs100% <100%> (ø)
... and 4 more
artidoro
artidoro
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StaticPipe/LbfgsStatic.cs
Outdated

 /// Binary Classification trainer estimators.
 /// </summary>
 public static class LbfgsBinaryClassificationStaticExtensions
 public static class LbfgsBinaryExtensions
@artidoroartidoroMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
artidoro
artidoro
approved these changes
Mar 20, 2019
View reviewed changes
@artidoroartidoro
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@yaeldekel
yaeldekel
commented
Mar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

Should we update the name of the entry point as well? #WontFix
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
@wschin
wschin
commented
Mar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
 [TlcModule.EntryPoint(Name = "Trainers.LogisticRegressionBinaryClassifier",

No for BC.
In reply to: 474926238 [](ancestors = 474926238)
Refers to: src/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs:407 in ba22e12. [](commit_id = ba22e12, deletion_comment = False)
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/Standard/LogisticRegression/LogisticRegression.cs
Outdated

 /// <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,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsLogisticRegressionTrainer [](start = 32, length = 30)
Can you call it LbfgsBinaryTrainer? #Resolved
@wschinwschinMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@Ivanidzo4kaIvanidzo4kaMar 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfilipi has comment on your issue, and I share it as well.

Putting word Regression without Binary in the trainer is confusing.

If I would saw name of this trainer I would assume it's part of regression task.

Which is not.
In reply to: 267475700 [](ancestors = 267475700,267460936)
@sfilipisfilipiMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Ivanidzo4ka
Ivanidzo4ka
reviewed
Mar 20, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs
Outdated

 /// </format>
 /// </example>
 public static LogisticRegressionBinaryTrainer LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
 public static LbfgsLogisticRegressionTrainer LbfgsLogisticRegressio(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
@Ivanidzo4kaIvanidzo4kaMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@wschinwschinMar 20, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think my changes violate his rule.
In reply to: 267478532 [](ancestors = 267478532)
@rogancarrrogancarrMar 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me nervous so close to ship date. Let's get consensus in #3016 before we make the change. Right now, I feel that the changes are inconsistent and unclear.
In reply to: 267481247 [](ancestors = 267481247,267478532)
rogancarr
rogancarr
reviewed
Mar 20, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
Outdated

 .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());
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LbfgsCalibrated [](start = 57, length = 15)
I don't think we should call it this. LogisticRegression is fine but LbfgsCalibrated is too esoteric. #Resolved
@rogancarrrogancarrMar 20, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not a calibrated linear model. It's fitting the to the logistic loss function. #Resolved
@wschinwschinMar 21, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it returns CalibratedModelParametersBase<LinearBinaryModelParameters, PlattCalibrator, I feel LbfgsCalibrated is fine. SDCA is doing the same. #Resolved
@rogancarrrogancarrMar 21, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschinwschinMar 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
rogancarr
rogancarr
reviewed
Mar 22, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/LogisticRegression.cs
 .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());
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about making the other calibrated linear trainers, like SDCA into XyzLogisticRegression(). #Resolved
@rogancarrrogancarrMar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add that at a later time. #Resolved
rogancarr
rogancarr
previously approved these changes
Mar 22, 2019
View reviewed changes
@rogancarrrogancarr
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
wschin
requested a review
from abgoswamMarch 22, 2019 20:57
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Here is my understanding of how we arrived at naming convention of Trainers/ModelParameters
This PR proposes to modify the existing APIs as follows :
  • Renames several MLContext names

    • LogisticRegression to LbfgsLogisticRegression
    • PoissonRegression to LbfgsPoissonRegression
  • Renames Class names

    • LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainer
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
  1. users may miss even seeing LogisticRegression if it has Lbfgs prefix .
  2. create more doubt in their minds . W can have LogisticRegression as the API name and use documentation to clarify what the API implements behind the scene. Doesn't seem right for API design to contain implementation details.
@eerhardt@sfilipi #Resolved
@Ivanidzo4ka
Ivanidzo4ka
self-requested a review
March 22, 2019 22:05
Ivanidzo4ka
Ivanidzo4ka
previously requested changes
Mar 22, 2019
View reviewed changes
@Ivanidzo4kaIvanidzo4ka
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me block it for now.

I don't think we have agreement among all of us regarding how positive this change is.
@eerhardt
eerhardt
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Member
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
I believe @wschin answered this in the top comment to the PR:
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.
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
@rogancarr
rogancarr
commented
Mar 22, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
I think that it would also make sense to convert the binary SDCA trainer to be called SdcaLogisticRegression if we make this change to Lbfgs. That way, they both get the "LR Brand Name". #Resolved
@abgoswam
abgoswam
commented
Mar 22, 2019•

edited

Loading

Copy link


Copy Markdown

Member
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.
  • The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?
  • Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this paper, which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to not include Lbfgs in the name of the API itself
  • Scikit-learn solves this by adding parameter called "solver"
solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem. #Resolved
@wschin
wschin
commented
Mar 23, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
* The PR proposes adding "LbfgsLogisticRegression" in API name itself to make it concrete that this particular API uses "Lbfgs" as the optimization algorithm. Is that the correct approach ?

LBFGS can mean LBFGS family algorithms.
* Are we sure we are using "LBFGS" as the optimizing algorithm ? The internal repo refers to this [paper](https://www.microsoft.com/en-us/research/publication/scalable-training-of-l1-regularized-log-linear-models/?from=http%3A%2F%2Fresearch.microsoft.com%2Fapps%2Fpubs%2Fdefault.aspx%3Fid%3D78900), which leads me to believe that the optimization algo that ML.NET uses is not LBFGS itself, but rather one of its variants "OWN-QN". All the more reason to _not_ include Lbfgs in the name of the API itself

It's a variant of LBFGS, as stated in your link.
* Scikit-learn [solves this](https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html) by adding parameter called "solver"

solver : str, {‘newton-cg’, ‘lbfgs’, ‘liblinear’, ‘sag’, ‘saga’}, default: ‘liblinear’.

Algorithm to use in the optimization problem.
No. It's not necessary. L1-norm --> a variant of LBFGS, No L1-norm --> LBFGS.
#Resolved
@abgoswam
abgoswam
commented
Mar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Couple of notes after discussing with @wschin
  • Since ML.NET is type-safe, we cannot really follow the same paradigm as Scikit-Learn's paradigm in the way it allows for specifying the optimization algorithm using the solver parameter.
  • We can take inspiration from Spark.ML to finilize the API
  • SDCA has other subtelities to it e.g. supports SVM loss, calibration/non-calibration etc. We can keep the SDCA API as is. Note that the current calibrated SDCA doesn't allow user to choose another loss function, so it always produces a logistic regression model. #Resolved
@Ivanidzo4ka
Ivanidzo4ka
dismissed
their stale reviewMarch 25, 2019 21:06
revoking review
abgoswam
abgoswam
approved these changes
Mar 25, 2019
View reviewed changes
@abgoswamabgoswam
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:
@wschin
Fix newly-added file
8af0d21
Remove empty line to trigger build
@wschinwschin
changed the title
Rename LogisticRegressionBinaryTrainer to LbfgsLogisticRegressionTrainerBetter names to calibreated linear classification modelsMar 25, 2019
@rogancarr
rogancarr
dismissed
their stale reviewMarch 25, 2019 21:25
revoking review
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
using Microsoft.ML.Transforms;

[assembly: LoadableClass(typeof(SymbolicSgdTrainer), typeof(SymbolicSgdTrainer.Options),
[assembly: LoadableClass(typeof(SymbolicSgdLogisticRegressionBinaryTrainer), typeof(SymbolicSgdLogisticRegressionBinaryTrainer.Options),
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SymbolicSgdLogisticRegressionBinaryTrainer [](start = 32, length = 42)
LogisticRegressionBinaryTrainer => LbfgsLogisticRegressionTrainer. Shall SymbolicSgdBinaryTrainer => SymbolicSgdLogisticRegressionTrainer instead of SymbolicSgdLogisticRegressionBinaryTrainer? Right now, the namings don't line up. #Resolved
@abgoswamabgoswamMar 25, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  • LbfgsLogisticRegressionBinaryTrainer
  • SymbolicSgdLogisticRegressionBinaryTrainer
In reply to: 268859710 [](ancestors = 268859710)
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our conclusion was having LR and Binary for trainer classes and only LR for APIs.
In reply to: 268865148 [](ancestors = 268865148,268859710)
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was mixing up catalogs & classes. #Resolved
rogancarr
rogancarr
reviewed
Mar 25, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs

 /// <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"/>.
@rogancarrrogancarrMar 25, 2019•

edited by wschin

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SdcaLogisticRegressionBinaryTrainer [](start = 89, length = 35)
Similar to SymSgd, now Sdca has LogisticRegression and Binary. #Resolved
@wschinwschinMar 25, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@wschin
wschin
merged commit 8730c87
into
dotnet:masterMar 25, 2019
@wschin
wschin
deleted the

sync-lbfgs-lr-me

branch
March 25, 2019 23:47
@zeahmedzeahmed
mentioned this pull request
Mar 29, 2019
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
@shmoradimsshmoradimsAwaiting requested review from shmoradims

4 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@artidoroartidoroartidoro approved these changes
@rogancarrrogancarrrogancarr approved these changes
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

APIIssues pertaining the friendly API

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@wschin@yaeldekel@abgoswam@eerhardt@rogancarr@Ivanidzo4ka@artidoro@sfilipi