Skip to content

Created samples for TokenizeIntoWords and RemoveStopWords APIs. #3156

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 8 commits into from
Apr 2, 2019

Conversation

zeahmed
Copy link
Contributor

@zeahmed zeahmed commented Apr 1, 2019

Related to #1209.

@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #3156 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3156      +/-   ##
==========================================
+ Coverage   72.53%   72.53%   +<.01%     
==========================================
  Files         808      807       -1     
  Lines      144775   144774       -1     
  Branches    16209    16208       -1     
==========================================
+ Hits       105012   105018       +6     
+ Misses      35348    35341       -7     
  Partials     4415     4415
Flag Coverage Δ
#Debug 72.53% <ø> (ø) ⬆️
#production 68.12% <ø> (ø) ⬆️
#test 88.82% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.Transforms/Text/TextCatalog.cs 41.66% <ø> (ø) ⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 86.1% <0%> (-0.16%) ⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs 86.03% <0%> (ø) ⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs 100% <0%> (ø) ⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs 93.49% <0%> (ø) ⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs 99.49% <0%> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/Training.cs 100% <0%> (ø) ⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs 98.55% <0%> (ø) ⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 100% <0%> (+20.51%) ⬆️
... and 1 more

@sfilipi sfilipi requested a review from natke April 1, 2019 21:09
var mlContext = new MLContext();

// Create an empty data sample list. The 'RemoveDefaultStopWords' does not require training data as
// the estimator ('StopWordsRemovingEstimator') created by 'RemoveDefaultStopWords' API is not a trainable estimator.
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Apr 1, 2019

Choose a reason for hiding this comment

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

StopWordsRemovingEstimator [](start = 31, length = 26)

any reason why you prefer this format other than <see cref="StopWordsRemovingEstimator"> ? #Pending

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 doubt if it works in normal comments. works in xml comments.


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

// as well as the source of randomness.
var mlContext = new MLContext();

// Create an empty data sample list. The 'RemoveDefaultStopWords' does not require training data as
Copy link
Contributor

@rogancarr rogancarr Apr 1, 2019

Choose a reason for hiding this comment

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

data sample list. [](start = 30, length = 18)

Create an empty list as the dataset. #Resolved

var emptyDataView = mlContext.Data.LoadFromEnumerable(emptySamples);

// A pipeline for converting text into vector of words.
var textPipeline = mlContext.Transforms.Text.TokenizeIntoWords("Words", "Text", separators: new[] { ' ' });
Copy link
Contributor

@rogancarr rogancarr Apr 1, 2019

Choose a reason for hiding this comment

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

separators: new[] { ' ' } [](start = 92, length = 25)

Can you add details about what to expect here with default values? One thing we may want to mention is removing any whitespace. #Resolved

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.

Approved with comments.

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:

@@ -254,8 +261,9 @@ public static class TextCatalog
/// <example>
/// <format type="text/markdown">
/// <![CDATA[
/// [!code-csharp[FastTree](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/StopWordRemoverTransform.cs)]
/// ]]></format>
/// [!code-csharp[RemoveStopWords](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/Text/RemoveDefaultStopWords.cs)]
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Apr 2, 2019

Choose a reason for hiding this comment

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

RemoveStopWords [](start = 26, length = 15)

RemoveDefaultStopWords
not sure is it important or not #Resolved

// A pipeline for removing stop words from input text/string.
// The pipeline first tokenizes text into words then removes stop words.
var textPipeline = mlContext.Transforms.Text.TokenizeIntoWords("Words", "Text")
.Append(mlContext.Transforms.Text.RemoveStopWords("WordsWithoutStopWords", "Words", stopwords: new[] { "a", "the", "from", "by" }));
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Apr 2, 2019

Choose a reason for hiding this comment

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

from [](start = 132, length = 4)

I would throw few words regarding casing.
imaging you have string "Something CoOl here" and you have stop word remover with "cool", would it remove it? Would it preserve it?
It's a mystery now, but you can show how it works in this sample. #Resolved

var emptyDataView = mlContext.Data.LoadFromEnumerable(emptySamples);

// A pipeline for removing stop words from input text/string.
// The pipeline first tokenizes text into words then removes stop words.
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Apr 2, 2019

Choose a reason for hiding this comment

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

removes [](start = 65, length = 7)

shall we add link to list of stop words?
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Transforms/Text/StopWords/English.txt this one is for english. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

also if you can modify string to include "tHe" or something like that and show it was removed (because we compare by ignoring casing (I hope so) would be nice.


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

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 think the link should go into the documentation instead of sample.


In reply to: 271478137 [](ancestors = 271478137,271477611)

@Ivanidzo4ka
Copy link
Contributor

        var data = new TextData() { Text = "This is a great product. I would like to buy it again."  };

It's me, your friendly casing neighbor.
In my example you can saw what xbox got treated even if we specify XBOX in file.


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/Text/ApplyCustomWordEmbedding.cs:54 in 1bc241d. [](commit_id = 1bc241d, deletion_comment = False)

@zeahmed zeahmed merged commit 950f133 into dotnet:master Apr 2, 2019
@zeahmed
Copy link
Contributor Author

zeahmed commented Apr 2, 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.

3 participants