Skip to content

Remove MD5Hasher. #2706

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
Feb 23, 2019
Merged

Remove MD5Hasher. #2706

merged 1 commit into from
Feb 23, 2019

Conversation

codemzs
Copy link
Member

@codemzs codemzs commented Feb 23, 2019

fixes #2696

@codemzs codemzs requested review from TomFinley and glebuk February 23, 2019 18:30
@codemzs codemzs self-assigned this Feb 23, 2019
@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2706      +/-   ##
==========================================
+ Coverage   71.59%   71.64%   +0.04%     
==========================================
  Files         805      804       -1     
  Lines      142004   141880     -124     
  Branches    16119    16111       -8     
==========================================
- Hits       101672   101651      -21     
+ Misses      35892    35792     -100     
+ Partials     4440     4437       -3
Flag Coverage Δ
#Debug 71.64% <ø> (+0.04%) ⬆️
#production 67.95% <ø> (+0.05%) ⬆️
#test 85.73% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...c/Microsoft.ML.FastTree/Dataset/SegmentIntArray.cs 0% <ø> (ø) ⬆️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs 33.91% <ø> (+1.89%) ⬆️
src/Microsoft.ML.FastTree/Dataset/DenseIntArray.cs 26.36% <ø> (+0.38%) ⬆️
...soft.ML.FastTree/BinFile/IniFileParserInterface.cs 0% <ø> (ø) ⬆️
...rc/Microsoft.ML.FastTree/Dataset/SparseIntArray.cs 54.16% <ø> (+0.14%) ⬆️
...rc/Microsoft.ML.FastTree/Dataset/RepeatIntArray.cs 0% <ø> (ø) ⬆️
src/Microsoft.ML.FastTree/Dataset/IntArray.cs 12.21% <ø> (ø) ⬆️
src/Microsoft.ML.FastTree/Dataset/Feature.cs 0% <ø> (ø) ⬆️
src/Microsoft.ML.Core/Data/ProgressReporter.cs 70.95% <0%> (-6.99%) ⬇️
src/Microsoft.ML.Maml/MAML.cs 24.75% <0%> (-1.46%) ⬇️
... and 1 more

@@ -254,8 +254,6 @@ public sealed class FeatureEvaluator

public int Id { get; }

public MD5Hash ContentMD5Hash => MD5Hasher.Hash(Content);
Copy link
Contributor

@glebuk glebuk Feb 23, 2019

Choose a reason for hiding this comment

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

ContentMD5Hash [](start = 27, length = 14)

Looks like the only place using it is DatasetBinFile3 in internal codebase. @TomFinley - ok to remove?

Copy link
Contributor

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

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you @codemzs !!

@TomFinley
Copy link
Contributor

I know @eerhardt had some interest in this, mentioning him as informational.

@codemzs codemzs merged commit eecf272 into dotnet:master Feb 23, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 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.

Need to remove FastTree/Utils/MD5Hasher class
3 participants