Skip to content

Microsoft.ML.Internal.Internallearn should be hidden/moved/renamed #2740

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 4 commits into from
Feb 27, 2019

Conversation

codemzs
Copy link
Member

@codemzs codemzs commented Feb 26, 2019

fixes #2714

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

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

:shipit:

@codecov
Copy link

codecov bot commented Feb 26, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2740      +/-   ##
==========================================
+ Coverage   71.66%   71.66%   +<.01%     
==========================================
  Files         808      808              
  Lines      142364   142364              
  Branches    16121    16121              
==========================================
+ Hits       102024   102030       +6     
+ Misses      35901    35898       -3     
+ Partials     4439     4436       -3
Flag Coverage Δ
#Debug 71.66% <ø> (ø) ⬆️
#production 67.91% <ø> (ø) ⬆️
#test 85.84% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...est/Microsoft.ML.Tests/FeatureContributionTests.cs 100% <ø> (ø) ⬆️
....Core.Tests/UnitTests/TestEarlyStoppingCriteria.cs 100% <ø> (ø) ⬆️
...dLearners/Standard/Online/OnlineGradientDescent.cs 91.3% <ø> (ø) ⬆️
test/Microsoft.ML.Predictor.Tests/TestIniModels.cs 100% <ø> (ø) ⬆️
...Microsoft.ML.Transforms/LearnerFeatureSelection.cs 0% <ø> (ø) ⬆️
...soft.ML.HalLearners/SymSgdClassificationTrainer.cs 74.69% <ø> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/Validation.cs 100% <ø> (ø) ⬆️
...est/Microsoft.ML.StaticPipelineTesting/Training.cs 99.25% <ø> (ø) ⬆️
...soft.ML.FastTree/Training/EarlyStoppingCriteria.cs 83.2% <ø> (ø)
...rosoft.ML.Data/Transforms/ExplainabilityCatalog.cs 100% <ø> (ø) ⬆️
... and 16 more

@TomFinley
Copy link
Contributor

TomFinley commented Feb 26, 2019

public interface IEarlyStoppingCriterion

Probably should not be an interface. #Resolved


Refers to: src/Microsoft.ML.Data/Training/EarlyStoppingCriteria.cs:29 in 07eb860. [](commit_id = 07eb860, deletion_comment = False)

Copy link
Member

@singlis singlis left a comment

Choose a reason for hiding this comment

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

:shipit:

@singlis
Copy link
Member

singlis commented Feb 26, 2019

Looks good pending the feedback from Tom. I agree if that is only used by FastTree (which looks to be only referenced in the BoostedTreeOptions), then I would move that code to the FastTree assembly. #Resolved

@codemzs
Copy link
Member Author

codemzs commented Feb 26, 2019

public interface IEarlyStoppingCriterion

Done.


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


Refers to: src/Microsoft.ML.Data/Training/EarlyStoppingCriteria.cs:29 in 07eb860. [](commit_id = 07eb860, deletion_comment = False)

@codemzs
Copy link
Member Author

codemzs commented Feb 26, 2019

thanks for reviewing. I have addressed Tom's comments.


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

@codemzs codemzs force-pushed the removeinternallearnns branch from 07eb860 to 7a23bbc Compare February 26, 2019 22:39
@codemzs codemzs force-pushed the removeinternallearnns branch from e9a757b to 720e938 Compare February 27, 2019 00:05
@codemzs codemzs merged commit 4eb9e45 into dotnet:master Feb 27, 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.

Microsoft.ML.Internal.Internallearn should be hidden/moved/renamed.
4 participants