Skip to content
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

Remove random trainer and model parameters from the public surface. #2849

Merged
merged 4 commits into from Mar 5, 2019

Conversation

Projects
None yet
3 participants
@TomFinley
Copy link
Contributor

commented Mar 5, 2019

Fix #2848.

@TomFinley TomFinley added the api label Mar 5, 2019

@TomFinley TomFinley self-assigned this Mar 5, 2019

@@ -9,6 +9,7 @@
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.StaticPipe" + PublicKey.Value)]
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Core.Tests" + PublicKey.TestValue)]
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Predictor.Tests" + PublicKey.TestValue)]
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Tests" + PublicKey.TestValue)]

This comment has been minimized.

Copy link
@TomFinley

TomFinley Mar 5, 2019

Author Contributor

I think @artidoro in his #2846 actually skipped this test, so we might be able to avoid this by simply removing the test; that the test was problematic was a side effect of merely being incorrect. Not sure what he thinks.

This comment has been minimized.

Copy link
@artidoro

artidoro Mar 5, 2019

Contributor

Give what you highlighted I think we should remove the test, we already have a test with TestEstimatorCore. If we want to make this estimator public, I think we should write tests in the new API anyway.

You can remove it here, or I can do it in my PR.

@@ -742,7 +742,7 @@ private static ICalibratorTrainer GetCalibratorTrainerOrThrow(IExceptionContext
/// [!code-csharp[FastTree](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/RandomTrainerSample.cs)]
/// ]]></format>

This comment has been minimized.

Copy link
@artidoro

artidoro Mar 5, 2019

Contributor

If you want to remove the sample I think you also need to remove the reference here.

@artidoro
Copy link
Contributor

left a comment

:shipit:

/// ]]></format>
/// </example>
public static RandomTrainer Random(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog)
internal static RandomTrainer Random(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog)

This comment has been minimized.

Copy link
@eerhardt

eerhardt Mar 5, 2019

Member

Can this just be deleted?

This comment has been minimized.

Copy link
@TomFinley

TomFinley Mar 5, 2019

Author Contributor

Yeah... actually from what I'm reading of the prior documentation it is simply not necessary at all. As far as I can tell, it existed in pre-IDataView days when evaluation didn't happen by evaluating predictions (which are of course just data), but evaluation happened over predictors (which in pre-IDataView days was the only mechanism we had that resembles what we now call ITransformer). Nowadays evaluation of course just takes an IDataView, so involving predictors is not necessary at all (and in fact merely complicates the whole story). Even from those days I can't even find any instances of that actually becoming useful, so while I was somewhat reluctant to undertake that drastic step, I think it probably is appropriate now that I've read a bit more about this.

This comment has been minimized.

Copy link
@TomFinley

TomFinley Mar 5, 2019

Author Contributor

Keeping the predictor for now for potential back-compat (we can figure out what we really want to do about that later), while as suggested getting rid of the trainer.

/// </summary>
public sealed class RandomModelParameters :
[Obsolete("This class was fundamnetally misdesigned, is incapable of implementing the interfaces it claims to implement, " +

This comment has been minimized.

Copy link
@eerhardt

eerhardt Mar 5, 2019

Member

type-o fundamnetally

@@ -9,6 +9,7 @@
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.StaticPipe" + PublicKey.Value)]
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Core.Tests" + PublicKey.TestValue)]
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Predictor.Tests" + PublicKey.TestValue)]
[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Tests" + PublicKey.TestValue)]

This comment has been minimized.

Copy link
@eerhardt

eerhardt Mar 5, 2019

Member

Do we still need this now?

This comment has been minimized.

Copy link
@TomFinley

TomFinley Mar 5, 2019

Author Contributor

Ah you're right.

@eerhardt
Copy link
Member

left a comment

:shipit:

@TomFinley TomFinley force-pushed the TomFinley:HideRandom branch from 9d49fb2 to b026984 Mar 5, 2019

@TomFinley TomFinley force-pushed the TomFinley:HideRandom branch from b026984 to 1a8146b Mar 5, 2019

@TomFinley TomFinley merged commit b70b424 into dotnet:master Mar 5, 2019

3 of 4 checks passed

MachineLearning-CodeCoverage #20190305.12 failed
Details
MachineLearning-CI #20190305.12 succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details

@TomFinley TomFinley deleted the TomFinley:HideRandom branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.