Skip to content

Scrub text featurizers #2944

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 1 commit into from
Mar 13, 2019
Merged

Scrub text featurizers #2944

merged 1 commit into from
Mar 13, 2019

Conversation

wschin
Copy link
Member

@wschin wschin commented Mar 13, 2019

Last step of #2832.

@wschin wschin self-assigned this Mar 13, 2019
@wschin wschin requested review from abgoswam and zeahmed March 13, 2019 18:15
@wschin wschin requested a review from sfilipi March 13, 2019 18:40
@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #2944 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2944      +/-   ##
==========================================
- Coverage   72.19%   72.18%   -0.01%     
==========================================
  Files         796      796              
  Lines      142023   142023              
  Branches    16046    16046              
==========================================
- Hits       102527   102521       -6     
- Misses      35116    35121       +5     
- Partials     4380     4381       +1
Flag Coverage Δ
#Debug 72.18% <87.5%> (-0.01%) ⬇️
#production 67.97% <85.71%> (-0.01%) ⬇️
#test 88.3% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...icrosoft.ML.Functional.Tests/DataTransformation.cs 100% <100%> (ø) ⬆️
...oft.ML.Transforms/Text/TextFeaturizingEstimator.cs 88.7% <85.71%> (ø) ⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 79.48% <0%> (-20.52%) ⬇️
src/Microsoft.ML.Maml/MAML.cs 24.75% <0%> (-1.46%) ⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.9% <0%> (+0.2%) ⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs 90.37% <0%> (+0.63%) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #2944 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2944      +/-   ##
==========================================
- Coverage   72.19%   72.18%   -0.01%     
==========================================
  Files         796      796              
  Lines      142023   142023              
  Branches    16046    16046              
==========================================
- Hits       102527   102521       -6     
- Misses      35116    35121       +5     
- Partials     4380     4381       +1
Flag Coverage Δ
#Debug 72.18% <87.5%> (-0.01%) ⬇️
#production 67.97% <85.71%> (-0.01%) ⬇️
#test 88.3% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...icrosoft.ML.Functional.Tests/DataTransformation.cs 100% <100%> (ø) ⬆️
...oft.ML.Transforms/Text/TextFeaturizingEstimator.cs 88.7% <85.71%> (ø) ⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 79.48% <0%> (-20.52%) ⬇️
src/Microsoft.ML.Maml/MAML.cs 24.75% <0%> (-1.46%) ⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.9% <0%> (+0.2%) ⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs 90.37% <0%> (+0.63%) ⬆️

[Argument(ArgumentType.AtMostOnce, HelpText = "Casing text using the rules of the invariant culture.", ShortName = "case", SortOrder = 5)]
public CaseMode TextCase = TextNormalizingEstimator.Defaults.Mode;
[Argument(ArgumentType.AtMostOnce, HelpText = "Casing text using the rules of the invariant culture.", Name="TextCase", ShortName = "case", SortOrder = 5)]
public CaseMode CaseMode = TextNormalizingEstimator.Defaults.Mode;
Copy link
Member

@abgoswam abgoswam Mar 13, 2019

Choose a reason for hiding this comment

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

CaseMode [](start = 28, length = 8)

does it make sense to call it TextCaseMode ? #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

This is CaseMode in TextFeaturizingEstimator, I'd say Text will look a bit redundant.


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

Copy link
Member

@abgoswam abgoswam 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

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

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

:shipit:

@wschin wschin merged commit d6c4872 into dotnet:master Mar 13, 2019
@wschin wschin deleted the polish-text-feat branch March 13, 2019 20:47
@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