Skip to content

Remove generic normalizer estimator catalog methods. #3116

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

Closed
wants to merge 2 commits into from

Conversation

codemzs
Copy link
Member

@codemzs codemzs commented Mar 27, 2019

fixes #3161

@codemzs codemzs requested review from wschin and artidoro March 27, 2019 20:07
@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #3116 into master will decrease coverage by <.01%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master    #3116      +/-   ##
==========================================
- Coverage   72.53%   72.53%   -0.01%     
==========================================
  Files         808      808              
  Lines      144740   144734       -6     
  Branches    16202    16201       -1     
==========================================
- Hits       104987   104979       -8     
- Misses      35343    35344       +1     
- Partials     4410     4411       +1
Flag Coverage Δ
#Debug 72.53% <97.56%> (-0.01%) ⬇️
#production 68.12% <0%> (-0.01%) ⬇️
#test 88.82% <100%> (ø) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.Transforms/NormalizerCatalog.cs 60% <ø> (+23.63%) ⬆️
...c/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs 24.46% <0%> (ø) ⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs 100% <100%> (ø) ⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs 98.55% <100%> (ø) ⬆️
...ios/IrisPlantClassificationWithStringLabelTests.cs 98.63% <100%> (ø) ⬆️
test/Microsoft.ML.Tests/OnnxConversionTest.cs 97.22% <100%> (ø) ⬆️
...soft.ML.Tests/PermutationFeatureImportanceTests.cs 100% <100%> (ø) ⬆️
...irectInstantiation/IrisPlantClassificationTests.cs 100% <100%> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/Training.cs 100% <100%> (ø) ⬆️
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs 100% <100%> (ø) ⬆️
... and 8 more

@codemzs codemzs requested review from Ivanidzo4ka and abgoswam March 29, 2019 17:59
@codemzs codemzs changed the title Cleanup unused method in Normalizer Estimator. Remove generic normalizer estimator catalog methods. Apr 1, 2019
@codemzs codemzs force-pushed the cleanupnormalizer branch from b660139 to 0c283e0 Compare April 1, 2019 23:23
using Microsoft.ML.Data;
using Microsoft.ML.Transforms;
using Microsoft.ML.Experimental;
Copy link
Member

Choose a reason for hiding this comment

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

Is it just experimental? We are removing the generic estimator, so these new methods must be supported officially and therefore deserve a namespace like ML.Transform. Is my understanding correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TomFinley
Copy link
Contributor

TomFinley commented Apr 2, 2019

As elsewhere noted, this should not be checked in independent of #3118 since, if it were to go in, it would leave the code in a bad state where there is no non-experimental means of normalizing columns. I believe you've correspondingly merged these changes in with #3118, so, close @codemzs ?

@codemzs codemzs closed this Apr 2, 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.

Remove generic normalizer estimator catalog methods.
4 participants