Add support for combining hashes in vector columns to HashingTransformer by yaeldekel · Pull Request #4828 · dotnet/machinelearning

yaeldekel · 2020-02-12T11:58:04Z

I am making this change so that CountTargetEncodingEstimator (see PR #4514 ) can start using HashingEstimator instead of HashJoiningTransform (see this comment in the above PR).

In addition to enabling CountTargetEncodingEstimator to use it, this will fix a bug when splitting datasets in the CV command and in the train-test/CV APIs. If a stratification column that is a vector type is specified, currently an exception will be thrown because the RangeFilter applied after hashing cannot handle vector columns.

yaeldekel · 2020-02-17T13:31:45Z

Question: some of the input types for hashing have missing values (float, double and key type). The hashing transformer maps missing values in the input, to 0, the missing value of key type. What should we do in the combine case?
I think the correct thing to do is to return 0 if any of the slots have missing values (that way, the behavior of a length-1 vector is identical whether Combine is true or false), but I would like to know what other people think.

codemzs · 2020-02-21T07:36:09Z

src/Microsoft.ML.Data/DataLoadSave/DataOperationsCatalog.cs

+                        columnOptions = new HashingEstimator.ColumnOptions(samplingKeyColumn, origStratCol, 30, (uint)seed.Value, combine: true);
                    else if (((ISeededEnvironment)env).Seed.HasValue)
-                        columnOptions = new HashingEstimator.ColumnOptionsInternal(samplingKeyColumn, origStratCol, 30, (uint)((ISeededEnvironment)env).Seed.Value);
+                        columnOptions = new HashingEstimator.ColumnOptions(samplingKeyColumn, origStratCol, 30, (uint)((ISeededEnvironment)env).Seed.Value, combine: true);


You can merge these conditions in one by defining var localSeed = seed.HasValue ? seed.Value : (ISeededEnvironment)env).Seed.HasValue ? (ISeededEnvironment)env).Seed.Value : null;

if(localSeed.HasValue)
...
else
columnOptions = new HashingEstimator.ColumnOptions(samplingKeyColumn, origStratCol, 30, combine: true); #Resolved

I don't think we want HashingEstimator listening to the global seed. The hashing is a different meaning of "seed"; this isn't a PRNG seed.

See #4752 (comment)

/cc @najeeb-kazmi

codemzs · 2020-02-21T07:41:38Z

src/Microsoft.ML.Data/Transforms/Hashing.cs

                modelSignature: "HASHTRNS",
                // verWrittenCur: 0x00010001, // Initial
-                verWrittenCur: 0x00010002, // Invert hash key values, hash fix
+                //verWrittenCur: 0x00010002, // Invert hash key values, hash fix


v [](start = 18, length = 1)

v [](start = 18, length = 1)

space #Resolved

harishsk · 2020-03-03T00:18:04Z

src/Microsoft.ML.Data/Commands/CrossValidationCommand.cs

+                        input = new TypeConvertingTransformer(Host, origStratCol, DataKind.Int64, origStratCol).Transform(input);
+
+                    output = new HashingEstimator(Host, stratificationColumn, origStratCol, 30).Fit(input).Transform(input);
                }


I realize the previous code too used 30 bits. But the default seems to be 31 bits. Any reason why we have 30 here? If there is a specific reason for 30 bits, can you please add a note to that in the code?

I am actually not sure why 30 was chosen. If you think it's important I can try changing it back to 31.

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

I have no reason to say it should change to 31 either :-). But since it is is used in a couple of places, can you please make it a named constant?

In reply to: 425102125 [](ancestors = 425102125,386729669)

harishsk · 2020-03-03T00:48:26Z

src/Microsoft.ML.Data/DataLoadSave/DataOperationsCatalog.cs

+                    var columnOptions =
+                        localSeed.HasValue ?
+                        new HashingEstimator.ColumnOptions(samplingKeyColumn, origStratCol, 30, (uint)localSeed.Value, combine: true) :
+                        new HashingEstimator.ColumnOptions(samplingKeyColumn, origStratCol, 30, combine: true);


Similar comment as before. If the 30bits hashing is standard, can we define it as some global constant somewhere?

harishsk · 2020-03-03T00:57:55Z

src/Microsoft.ML.Data/Transforms/Hashing.cs

                return ComposeGetterOne(input, iinfo, srcCol, srcType);
+            if (_columns[iinfo].Combine)
+                return ComposeGetterCombined(input, iinfo, srcCol, vectorType);
            return ComposeGetterVec(input, iinfo, srcCol, vectorType);


We need to figure out the onnx export mechanism for this. #Resolved

For now I'm making this case non-exportable.

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

…y of HashingEstimator.

…test for it.

harishsk · 2020-05-13T16:19:05Z

src/Microsoft.ML.Data/Transforms/Hashing.cs

-                verReadableCur: 0x00010003,
+                //verWrittenCur: 0x00010003, // Uses MurmurHash3_x86_32
+                verWrittenCur: 0x00010004, // Add Combine option
+                verReadableCur: 0x00010004,


We still haven't released a version with MurmurHash3. Do we need to increment this if we make sure to include it in the v1.5 release?

Technically users may create and save models containing this transformer once the PR that changed the version to 0x00010003 was merged. This most likely didn't happen, but to be on the safe side I think it's better to increment the version again.

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

harishsk · 2020-05-13T16:21:17Z

src/Microsoft.ML.Data/Transforms/Hashing.cs

        private readonly VectorDataViewType[] _kvTypes;
-        private readonly bool _isVersion1;
+        private readonly bool _useOldHashing;



suggestion: Is it possible to come up with a better name? The only one I can think of is "_useOnnxCompatibleHashing" which would invert the meaning of the current name. Please use a a more descriptive name if possible.

I would like to keep it a variable that would have the value "false" by default, that's why I didn't want to change it to _useOnnxCompatibleHashing. Is this name that I changed it to acceptable?

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

harishsk · 2020-05-13T16:47:57Z

src/Microsoft.ML.Data/Transforms/Hashing.cs

+                if (srcType is KeyDataViewType)
+                    return false;
+                if (_parent._columns[iinfo].Combine)
                    return false;


@lynx1820 Any thoughts why we are not supporting key types? #Resolved

Thanks for the remainder! I'll work on adding support for keytypes. Zero value keys don't get mapped to a hash but map to zero, a behavior not currently present. #Resolved

As @lynx1820 mentioned, in key types 0 is mapped to 0 instead of to its hash value, so it would be more complicated to export to ONNX.

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

test/Microsoft.ML.Tests/Transformers/HashTests.cs

codecov · 2020-05-14T09:49:22Z

Codecov Report

Merging #4828 into master will decrease coverage by 2.16%.
The diff coverage is 75.35%.

@@            Coverage Diff             @@
##           master    #4828      +/-   ##
==========================================
- Coverage   75.71%   73.54%   -2.17%     
==========================================
  Files         993      992       -1     
  Lines      179035   179484     +449     
  Branches    19356    19296      -60     
==========================================
- Hits       135549   131997    -3552     
- Misses      38262    42305    +4043     
+ Partials     5224     5182      -42

Flag	Coverage Δ
#Debug	`73.54% <75.35%> (-2.17%)`	⬇️
#production	`68.84% <68.71%> (-2.85%)`	⬇️
#test	`88.67% <85.67%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
test/Microsoft.ML.Benchmarks/HashBench.cs	`0.00% <0.00%> (ø)`
src/Microsoft.ML.EntryPoints/TrainTestSplit.cs	`76.92% <45.00%> (-16.11%)`	⬇️
...L.Transforms/Text/WordHashBagProducingTransform.cs	`61.08% <50.00%> (ø)`
test/Microsoft.ML.Tests/DatabaseLoaderTests.cs	`94.07% <52.63%> (ø)`
...crosoft.ML.Data/Commands/CrossValidationCommand.cs	`83.52% <60.00%> (+2.04%)`	⬆️
src/Microsoft.ML.Data/Transforms/Hashing.cs	`81.38% <69.15%> (-7.58%)`	⬇️
...soft.ML.Data/DataLoadSave/DataOperationsCatalog.cs	`81.81% <75.00%> (+4.64%)`	⬆️
test/Microsoft.ML.Tests/TextLoaderTests.cs	`95.53% <77.77%> (ø)`
.../Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs	`82.21% <100.00%> (ø)`
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs	`71.84% <100.00%> (-0.54%)`	⬇️
... and 120 more

codecov · 2020-05-14T10:53:45Z

Codecov Report

Merging #4828 into master will decrease coverage by 0.10%.
The diff coverage is 75.33%.

@@            Coverage Diff             @@
##           master    #4828      +/-   ##
==========================================
- Coverage   75.71%   75.60%   -0.11%     
==========================================
  Files         993      995       +2     
  Lines      179035   179664     +629     
  Branches    19356    19314      -42     
==========================================
+ Hits       135549   135834     +285     
- Misses      38262    38565     +303     
- Partials     5224     5265      +41

Flag	Coverage Δ
#Debug	`75.60% <75.33%> (-0.11%)`	⬇️
#production	`71.55% <68.71%> (-0.14%)`	⬇️
#test	`88.66% <85.63%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
test/Microsoft.ML.Benchmarks/HashBench.cs	`0.00% <0.00%> (ø)`
src/Microsoft.ML.EntryPoints/TrainTestSplit.cs	`76.92% <45.00%> (-16.11%)`	⬇️
...L.Transforms/Text/WordHashBagProducingTransform.cs	`61.08% <50.00%> (ø)`
test/Microsoft.ML.Tests/DatabaseLoaderTests.cs	`94.07% <52.63%> (ø)`
...crosoft.ML.Data/Commands/CrossValidationCommand.cs	`83.52% <60.00%> (+2.04%)`	⬆️
src/Microsoft.ML.Data/Transforms/Hashing.cs	`81.38% <69.15%> (-7.58%)`	⬇️
...soft.ML.Data/DataLoadSave/DataOperationsCatalog.cs	`81.81% <75.00%> (+4.64%)`	⬆️
test/Microsoft.ML.Tests/TextLoaderTests.cs	`95.53% <77.77%> (ø)`
.../Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs	`82.21% <100.00%> (ø)`
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs	`71.84% <100.00%> (-0.54%)`	⬇️
... and 57 more

harishsk · 2020-05-14T23:49:23Z

That sounds good to me.

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

harishsk

yaeldekel requested a review from a team as a code owner February 12, 2020 11:58

yaeldekel force-pushed the hashing branch from f4229bb to 249db91 Compare February 12, 2020 12:42

codemzs reviewed Feb 21, 2020

View reviewed changes

yaeldekel requested a review from a team as a code owner February 25, 2020 22:13

yaeldekel force-pushed the hashing branch 2 times, most recently from bfd1524 to f62a846 Compare February 26, 2020 21:44

harishsk reviewed Mar 3, 2020

View reviewed changes

yaeldMS added 6 commits May 6, 2020 09:39

Add "Combine" option.

03fbcf0

Add tests.

3d332ce

Add tests, and change data splitting APIs to use the new functionalit…

ea4a0b8

…y of HashingEstimator.

Address code review comment, change missing value handling and add a …

08bb1e5

…test for it.

Fix hashing tests.

d48a3e8

Add test for backwards compatibility of model format.

7868a52

yaeldekel force-pushed the hashing branch from 7709a8e to 7868a52 Compare May 6, 2020 06:43

yaeldMS added 5 commits May 6, 2020 10:58

Fix build breaks and tests after rebase.

f9c13aa

Combine V2 and V1 implementations of IHashers into one.

d315999

Update SaveAsOnnx.

ef735d9

fix baseline

d2c2d6a

More unit test fixes

d05dcf0

harishsk reviewed May 13, 2020

View reviewed changes

kere-nel reviewed May 13, 2020

View reviewed changes

test/Microsoft.ML.Tests/Transformers/HashTests.cs Outdated Show resolved Hide resolved

yaeldMS added 2 commits May 14, 2020 12:33

Code review comments.

c5a140f

Add back the back-compat test.

e6bb9d8

harishsk approved these changes May 14, 2020

View reviewed changes

yaeldekel merged commit 6fddd31 into dotnet:master May 18, 2020

yaeldekel deleted the hashing branch May 18, 2020 06:26

antoniovs1029 mentioned this pull request Jun 10, 2020

CrossValidation Macros stops working in 1.5.0 #5221

Closed

ghost locked as resolved and limited conversation to collaborators Mar 19, 2022

Conversation

yaeldekel commented Feb 12, 2020

Uh oh!

yaeldekel commented Feb 17, 2020

Uh oh!

codemzs Feb 21, 2020 • edited by yaeldekel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Feb 21, 2020 • edited by yaeldekel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harishsk Mar 3, 2020 • edited by yaeldekel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harishsk May 13, 2020 • edited by yaeldekel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kere-nel May 13, 2020 • edited by yaeldekel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented May 14, 2020

Codecov Report

Uh oh!

codecov bot commented May 14, 2020

Codecov Report

Uh oh!

harishsk commented May 14, 2020

Uh oh!

harishsk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codemzs Feb 21, 2020 •

edited by yaeldekel

Loading

codemzs Feb 21, 2020 •

edited by yaeldekel

Loading

harishsk Mar 3, 2020 •

edited by yaeldekel

Loading

harishsk May 13, 2020 •

edited by yaeldekel

Loading

kere-nel May 13, 2020 •

edited by yaeldekel

Loading