Skip to content

Updating .tt file due to renaming of LbfgsPoissonRegression. #3140

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 3 commits into from
Apr 1, 2019

Conversation

zeahmed
Copy link
Contributor

@zeahmed zeahmed commented Mar 29, 2019

This PR fixes the issue related to renaming done in #3034. Currently, we are finding a way to forcing people to update the .tt file instead of .cs file generated from .tt.

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #3140 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3140      +/-   ##
==========================================
+ Coverage   72.52%   72.53%   +0.01%     
==========================================
  Files         808      808              
  Lines      144665   144740      +75     
  Branches    16198    16202       +4     
==========================================
+ Hits       104912   104987      +75     
- Misses      35342    35343       +1     
+ Partials     4411     4410       -1
Flag Coverage Δ
#Debug 72.53% <ø> (+0.01%) ⬆️
#production 68.12% <ø> (ø) ⬆️
#test 88.82% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs 89.07% <ø> (ø) ⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs 97.26% <0%> (-2.74%) ⬇️
...rc/Microsoft.ML.StaticPipe/SdcaStaticExtensions.cs 81.72% <0%> (-0.61%) ⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.7% <0%> (-0.21%) ⬇️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs 41.66% <0%> (ø) ⬆️
...oft.ML.StandardTrainers/Standard/SdcaRegression.cs 95.83% <0%> (ø) ⬆️
...crosoft.ML.StandardTrainers/Standard/SdcaBinary.cs 72.95% <0%> (+0.17%) ⬆️
...c/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs 24.46% <0%> (+0.73%) ⬆️
src/Microsoft.ML.Maml/MAML.cs 26.21% <0%> (+1.45%) ⬆️
...StandardTrainers/Standard/StochasticTrainerBase.cs 93.02% <0%> (+4.65%) ⬆️

@abgoswam
Copy link
Member

abgoswam commented Mar 29, 2019

    ///  [!code-csharp[PoissonRegression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/PoissonRegressionWithOptions.cs)]

should we fix this as well ? #Resolved


Refers to: src/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs:596 in 430a999. [](commit_id = 430a999, deletion_comment = False)

@abgoswam
Copy link
Member

abgoswam commented Mar 29, 2019

    ///  [!code-csharp[PoissonRegression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/PoissonRegressionWithOptions.cs)]

fix this too ? #Resolved


Refers to: src/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs:596 in 430a999. [](commit_id = 430a999, deletion_comment = False)

@@ -567,7 +567,7 @@ public static LbfgsLogisticRegressionBinaryTrainer LbfgsLogisticRegression(this
/// <example>
/// <format type="text/markdown">
/// <![CDATA[
/// [!code-csharp[PoissonRegression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/PoissonRegression.cs)]
/// [!code-csharp[PoissonRegression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/LbfgsPoissonRegression.cs)]
Copy link
Member

@abgoswam abgoswam Mar 29, 2019

Choose a reason for hiding this comment

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

PoissonRegression [](start = 27, length = 17)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need to prefix this with Lbfgs.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the alt-text for the link, so it should be the name of the API.


In reply to: 270973957 [](ancestors = 270973957,270581846)

@@ -567,7 +567,7 @@ public static LbfgsLogisticRegressionBinaryTrainer LbfgsLogisticRegression(this
/// <example>
/// <format type="text/markdown">
/// <![CDATA[
/// [!code-csharp[PoissonRegression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/PoissonRegression.cs)]
/// [!code-csharp[PoissonRegression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/LbfgsPoissonRegression.cs)]
/// ]]></format>
/// </example>
public static LbfgsPoissonRegressionTrainer LbfgsPoissonRegression(this RegressionCatalog.RegressionTrainers catalog,
Copy link
Member

@abgoswam abgoswam Mar 29, 2019

Choose a reason for hiding this comment

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

LbfgsPoissonRegressionTrainer [](start = 22, length = 29)

there were other renamings which accompanied this .. should we fix those too in this PR ?

#3016 (comment) #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rest of the examples are not using .tt or they have been converted to .tt after the above change. So, those are fine right now.


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

<Generator>TextTemplatingFileGenerator</Generator>
<LastGenOutput>PoissonRegression.cs</LastGenOutput>
<LastGenOutput>LbfgsPoissonRegression.cs</LastGenOutput>
</None>
<None Update="Dynamic\Trainers\Regression\PoissonRegressionWithOptions.tt">
Copy link

@shmoradims shmoradims Mar 29, 2019

Choose a reason for hiding this comment

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

PoissonRegressionWithOptions [](start = 46, length = 28)

please also fix this in the same way #Closed

Copy link

@shmoradims shmoradims left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@singlis singlis left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

Appoved with comments.

@zeahmed zeahmed merged commit 41a6308 into dotnet:master Apr 1, 2019
@zeahmed
Copy link
Contributor Author

zeahmed commented Apr 1, 2019

Thanks!

zeahmed added a commit to zeahmed/machinelearning that referenced this pull request Apr 8, 2019
@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.

5 participants