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

Move IModelCombiner to Microsoft.ML.Data and Lockdown Microsoft.ML.Ensemble #2505

Merged
merged 7 commits into from Feb 18, 2019

Conversation

Projects
None yet
3 participants
@yaeldekel
Copy link
Member

yaeldekel commented Feb 11, 2019

We have an implementation of IModelCombiner in Microsoft.ML.FastTree. This PR is to move the interface to Microsoft.ML.Data so that Microsoft.ML.FastTree doesn't need to reference Microsoft.ML.Ensemble.

This PR also internalizes most of the classes in Microsoft.ML.Ensemble. Fixes #2268 .

@yaeldekel yaeldekel requested review from codemzs and TomFinley Feb 11, 2019

@Ivanidzo4ka

This comment has been minimized.

Copy link
Member

Ivanidzo4ka commented Feb 12, 2019

public sealed class TreeEnsembleCombiner : IModelCombiner

shall we make it internal as well? #Resolved


Refers to: src/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsembleCombiner.cs:15 in a167c4d. [](commit_id = a167c4d, deletion_comment = False)

@Ivanidzo4ka
Copy link
Member

Ivanidzo4ka left a comment

:shipit:

yaeldekel added some commits Feb 12, 2019

@@ -12,7 +12,7 @@

namespace Microsoft.ML.Trainers.Ensemble
{
public sealed class Average : BaseAverager, IRegressionOutputCombiner
internal sealed class Average : BaseAverager, IRegressionOutputCombiner

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Feb 13, 2019

Member

Do we have any tests/ examples of how use Ensembles? I'm just curious how user will set any of combiners if they all hidden

This comment has been minimized.

@yaeldekel

yaeldekel Feb 13, 2019

Author Member

The current API for ensembles is not the one we want to make public, so I am internalizing it until we get a chance to develop a good public API for it.


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

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Feb 13, 2019

Member

So if everything in this Ensemble is hidden, and we don't have any API exposing Ensembles shall we exclude it from Microsoft.ML package? See Microsoft.ML.Ensemble.csproj Microsoft.ML
Would it break anything?


In reply to: 256514419 [](ancestors = 256514419,256207741)

This comment has been minimized.

@yaeldekel

yaeldekel Feb 13, 2019

Author Member

Hmm. Good question. If there are any consumers of the nuget that use maml or entry points then we should probably leave it.


In reply to: 256542187 [](ancestors = 256542187,256514419,256207741)

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 13, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2505      +/-   ##
==========================================
+ Coverage   71.25%   71.25%   +<.01%     
==========================================
  Files         798      798              
  Lines      141252   141252              
  Branches    16112    16112              
==========================================
+ Hits       100643   100644       +1     
+ Misses      36145    36144       -1     
  Partials     4464     4464
Flag Coverage Δ
#Debug 71.25% <ø> (ø) ⬆️
#production 67.58% <ø> (ø) ⬆️
#test 85.35% <ø> (ø) ⬆️
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 13, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@e3b5f76). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2505   +/-   ##
=========================================
  Coverage          ?    71.5%           
=========================================
  Files             ?      801           
  Lines             ?   142033           
  Branches          ?    16148           
=========================================
  Hits              ?   101557           
  Misses            ?    36004           
  Partials          ?     4472
Flag Coverage Δ
#Debug 71.5% <ø> (?)
#production 67.78% <ø> (?)
#test 85.56% <ø> (?)

@Ivanidzo4ka Ivanidzo4ka changed the title Move IModelCombiner to Microsoft.ML.Data Move IModelCombiner to Microsoft.ML.Data and Lockdown Microsoft.ML.Ensemble Feb 15, 2019

@TomFinley
Copy link
Contributor

TomFinley left a comment

Thanks @yaeldekel

@TomFinley TomFinley merged commit b881fdc into dotnet:master Feb 18, 2019

3 checks passed

MachineLearning-CI #20190218.1 succeeded
Details
MachineLearning-CodeCoverage #20190218.1 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment