Skip to content
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

Adding support for MurmurHash KeyDataTypes #5138

Merged
merged 3 commits into from
May 21, 2020

Conversation

Lynx1820
Copy link
Contributor

ML.NET's behavior is to map zero input values to zero, instead of hashes.

  • Adding that behavior to the onnx export and a test.

TODO: rebase once other murmurhash PR are merged

@Lynx1820 Lynx1820 requested a review from a team as a code owner May 16, 2020 00:27
@codecov
Copy link

codecov bot commented May 16, 2020

Codecov Report

Merging #5138 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5138      +/-   ##
==========================================
+ Coverage   75.66%   75.68%   +0.01%     
==========================================
  Files         990      990              
  Lines      179815   179867      +52     
  Branches    19342    19346       +4     
==========================================
+ Hits       136061   136134      +73     
+ Misses      38482    38464      -18     
+ Partials     5272     5269       -3     
Flag Coverage Δ
#Debug 75.68% <100.00%> (+0.01%) ⬆️
#production 71.57% <100.00%> (+0.01%) ⬆️
#test 88.93% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.Data/Transforms/Hashing.cs 82.16% <100.00%> (+0.70%) ⬆️
test/Microsoft.ML.Tests/OnnxConversionTest.cs 99.45% <100.00%> (+0.01%) ⬆️
src/Microsoft.ML.Featurizers/CategoricalImputer.cs 74.73% <0.00%> (-0.27%) ⬇️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs 86.89% <0.00%> (+1.74%) ⬆️
...rosoft.ML.AutoML/ColumnInference/TextFileSample.cs 62.91% <0.00%> (+3.31%) ⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 100.00% <0.00%> (+20.51%) ⬆️

@harishsk
Copy link
Contributor

            }

What other datatype are we not supporting? Can we assert here instead?


Refers to: src/Microsoft.ML.Data/Transforms/Hashing.cs:1162 in 21ad377. [](commit_id = 21ad377, deletion_comment = False)

@Lynx1820
Copy link
Contributor Author

            }

What other datatype are we not supporting? Can we assert here instead?

Refers to: src/Microsoft.ML.Data/Transforms/Hashing.cs:1162 in 21ad377. [](commit_id = 21ad377, deletion_comment = False)

All numeric types are now supported.

Copy link
Contributor

@harishsk harishsk left a comment

Choose a reason for hiding this comment

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

:shipit:

@harishsk harishsk merged commit d58e8d1 into dotnet:master May 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 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.

2 participants