Skip to content

Conversation

@TomFinley
Copy link
Contributor

Towards but not quite finishing #2251. Why not finish? At issue is that we must resolve #2378 in some fashion (TBD) before we continue, but as it is I think this is a move in the right direction. The public surface using this interface has now been reduced considerably, but of course not quite completely eliminated. So this is not a work in progress, but I didn't want to leave such a big change lying around.

First commit internalizes the (less used) IDistPredictorProducing, while the second does some but not all of the work of internalizing IPredictorProducing.

@TomFinley TomFinley added the API Issues pertaining the friendly API label Feb 4, 2019
@TomFinley TomFinley self-assigned this Feb 4, 2019
@codecov
Copy link

codecov bot commented Feb 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1706b3c). Click here to learn what that means.
The diff coverage is 71.73%.

@@            Coverage Diff            @@
##             master    #2404   +/-   ##
=========================================
  Coverage          ?   71.25%           
=========================================
  Files             ?      785           
  Lines             ?   140869           
  Branches          ?    16102           
=========================================
  Hits              ?   100381           
  Misses            ?    36023           
  Partials          ?     4465
Flag Coverage Δ
#Debug 71.25% <71.73%> (?)
#production 67.61% <67.5%> (?)
#test 85.3% <100%> (?)

@TomFinley TomFinley requested a review from wschin February 4, 2019 20:54
ConcurrentDictionary<FeatureSubsetModel<TOutput>, TOutput[]> predictions);
}

public delegate void SignatureEnsembleDiversityMeasure();
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Feb 4, 2019

Choose a reason for hiding this comment

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

public [](start = 4, length = 6)

public [](start = 4, length = 6)

maybe hide it as well? #Closed

@Ivanidzo4ka
Copy link
Contributor

using TScalarPredictor = IPredictorProducing<Single>;

you can remove this one


Refers to: src/Microsoft.ML.Ensemble/Trainer/EnsembleModelParameters.cs:22 in ba51e48. [](commit_id = ba51e48, deletion_comment = False)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka 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
Member

@wschin wschin left a comment

Choose a reason for hiding this comment

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

A super cool PR!

* Internalize the interface IDistPredictorProducing.
* Remove the two-generic IPredictor as it is unused and unusable.
* Refactor much of the Ensemble code so that it uses IPredictorProducing less in its public surface.
@TomFinley TomFinley merged commit 6b1a0d3 into dotnet:master Feb 5, 2019
@TomFinley TomFinley deleted the HidePred branch February 11, 2019 20:22
@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

API Issues pertaining the friendly API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

State of CalibratorPredictorBase v1

3 participants