Skip to content

Scrubbing SDCA learners #2825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 6, 2019
Merged

Conversation

abgoswam
Copy link
Member

@abgoswam abgoswam commented Mar 3, 2019

Fixes #2616 . Related #2613

@codecov
Copy link

codecov bot commented Mar 3, 2019

Codecov Report

Merging #2825 into master will increase coverage by <.01%.
The diff coverage is 92.95%.

@@            Coverage Diff             @@
##           master    #2825      +/-   ##
==========================================
+ Coverage   71.69%    71.7%   +<.01%     
==========================================
  Files         809      810       +1     
  Lines      142444   142473      +29     
  Branches    16109    16111       +2     
==========================================
+ Hits       102130   102156      +26     
- Misses      35889    35891       +2     
- Partials     4425     4426       +1
Flag Coverage Δ
#Debug 71.7% <92.95%> (ø) ⬆️
#production 67.92% <90.41%> (-0.01%) ⬇️
#test 85.91% <95.65%> (+0.01%) ⬆️
Impacted Files Coverage Δ
...StandardLearners/Standard/LinearModelParameters.cs 60.63% <ø> (-0.27%) ⬇️
...oft.ML.StandardLearners/Standard/SdcaRegression.cs 96.05% <ø> (ø) ⬆️
...t/Microsoft.ML.Benchmarks/PredictionEngineBench.cs 0% <0%> (ø) ⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs 100% <100%> (ø) ⬆️
...ML.Tests/Scenarios/Api/Estimators/Extensibility.cs 100% <100%> (ø)
...oft.ML.StandardLearners/StandardLearnersCatalog.cs 88.37% <100%> (ø) ⬆️
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs 92.5% <100%> (ø) ⬆️
...ios/IrisPlantClassificationWithStringLabelTests.cs 98.63% <100%> (ø) ⬆️
...cenarios/Api/Estimators/MultithreadedPrediction.cs 100% <100%> (ø) ⬆️
...L.Tests/Scenarios/Api/Estimators/Metacomponents.cs 100% <100%> (ø) ⬆️
... and 22 more


/// <summary>
/// Whether to shuffle data at every epoch (default true).
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 3, 2019

Choose a reason for hiding this comment

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

epoch [](start = 49, length = 5)

iteration? or it's two different things? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

retaining the latest documentation updates being done in parallel


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

/// <summary>
/// Convergence check frequency (in terms of <see cref="NumberOfIterations"/>). Set as negative or zero for not checking at all.
/// If left blank, it defaults to check after every <see cref="NumberOfThreads"/> iterations.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Convergence check frequency (in terms of number of iterations). Set as negative or zero for not checking at all. If left blank, it defaults to check after every 'numThreads' iterations.", NullName = "<Auto>", ShortName = "checkFreq")]
public int? CheckFrequency;
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckFrequency [](start = 24, length = 14)

Maybe ConvergenceCheckFrequency?
Also can you copy this documentation across other CheckFrequency options?

[TGUI(Label = "Initial Learning Rate (for SGD)")]
public double InitLearningRate = Defaults.InitLearningRate;
public double InitialLearningRate = Defaults.InitialLearningRate;
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 4, 2019

Choose a reason for hiding this comment

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

InitialLearningRate [](start = 26, length = 19)

So it works this way: in the beginning of each iteration we set learning rate to this one, and then with going through examples we decrease it. But in each iteration it starts with this one.
So technically it's initial learning rate, but it remain same for each iteration....
Can you look other example of InitialLearningRate, and are they have same pattern or not?
I would probably just call it LearningRate. #Resolved

Copy link
Member Author

@abgoswam abgoswam Mar 6, 2019

Choose a reason for hiding this comment

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

I do not see other examples of InitialLearningRate in the other learners.

To me the names makes sense, esp. since it does indicate that this learning rate is adaptive in nature. We can add more documentation as to why this helps . @shmoradims


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

@@ -26,7 +26,7 @@ namespace Microsoft.ML.Trainers
{
// SDCA linear multiclass trainer.
/// <include file='doc.xml' path='doc/members/member[@name="SDCA"]/*' />
public class SdcaMultiClassTrainer : SdcaTrainerBase<SdcaMultiClassTrainer.Options, MulticlassPredictionTransformer<MulticlassLogisticRegressionModelParameters>, MulticlassLogisticRegressionModelParameters>
public sealed class SdcaMultiClassTrainer : SdcaTrainerBase<SdcaMultiClassTrainer.Options, MulticlassPredictionTransformer<MulticlassLogisticRegressionModelParameters>, MulticlassLogisticRegressionModelParameters>
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 4, 2019

Choose a reason for hiding this comment

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

sealed [](start = 11, length = 6)

How I can put thumbs up emoji?
Anyway thanks for catching this! #Resolved

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@abgoswam abgoswam requested a review from artidoro March 4, 2019 18:12
int maxIterations = SgdBinaryTrainer.Options.Defaults.MaxIterations,
double initLearningRate = SgdBinaryTrainer.Options.Defaults.InitLearningRate,
float l2Weight = SgdBinaryTrainer.Options.Defaults.L2Weight)
int maxIterations = SgdBinaryTrainer.Options.Defaults.NumberOfIterations,
Copy link
Contributor

Choose a reason for hiding this comment

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

maxIterations [](start = 16, length = 13)

Should you reflect the change in the extensions as well?

Copy link
Contributor

@artidoro artidoro left a comment

Choose a reason for hiding this comment

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

:shipit:

@Ivanidzo4ka
Copy link
Contributor

@abgoswam Any chance you plan to update and merge this one?

@abgoswam abgoswam merged commit 002d6b4 into dotnet:master Mar 6, 2019
@abgoswam abgoswam deleted the abgoswam/scrub_SDCA branch March 20, 2019 20:13
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants