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

Creation of components through MLContext, internalization, and renaming #2510

Merged
merged 7 commits into from
Feb 13, 2019

Conversation

artidoro
Copy link
Contributor

Fixes #1798, #1758

  1. In this PR I internalize the constructors of DnnImageFeaturizerEstimator, PriorTrainer, and RandomTrainer. This required to add catalog extensions for these three trainers. Internalizing these constructor closes issue Creation of components through MLContext: advanced options and other feedback #1798.
  2. I also rename a few instances of Arguments to Options which closes issue Arguments class should be made internal when possible #1758.
  3. I internalized some fields across ITransformers and IEstimators.
  4. I also moved the Options class in TensorflowTransform to the estimator.

@artidoro artidoro self-assigned this Feb 12, 2019
@artidoro artidoro added the API Issues pertaining the friendly API label Feb 12, 2019
@@ -197,7 +197,7 @@ public sealed class CustomMappingEstimator<TSrc, TDst> : TrivialEstimator<Custom
/// <param name="contractName">The contract name, used by ML.NET for loading the model. If <c>null</c> is specified, such a trained model would not be save-able.</param>
/// <param name="inputSchemaDefinition">Additional parameters for schema mapping between <typeparamref name="TSrc"/> and input data.</param>
/// <param name="outputSchemaDefinition">Additional parameters for schema mapping between <typeparamref name="TDst"/> and output data.</param>
public CustomMappingEstimator(IHostEnvironment env, Action<TSrc, TDst> mapAction, string contractName,
internal CustomMappingEstimator(IHostEnvironment env, Action<TSrc, TDst> mapAction, string contractName,
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Feb 12, 2019

Choose a reason for hiding this comment

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

CustomMappingEstimator [](start = 17, length = 22)

We don't have any samples/ tests with it at all? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am adding a sample now.


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

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Feb 12, 2019

    public override SchemaShape GetOutputSchema(SchemaShape inputSchema)

you used to add summary comments in previous PRs #Resolved


Refers to: src/Microsoft.ML.Transforms/CustomMappingTransformer.cs:207 in 35e93ef. [](commit_id = 35e93ef, deletion_comment = False)

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Feb 12, 2019

    public RandomModelParameters(IHostEnvironment env, int seed)

I think we trying to close our current predictors till 1.0 #Resolved


Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:142 in 35e93ef. [](commit_id = 35e93ef, deletion_comment = False)

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Feb 12, 2019

    public PriorModelParameters(IHostEnvironment env, float prob)

internal? #Resolved


Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:379 in 35e93ef. [](commit_id = 35e93ef, 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:

@abgoswam
Copy link
Member

// Licensed to the .NET Foundation under one or more agreements.

no changes in this file.


Refers to: src/Microsoft.ML.Data/Prediction/CalibratorCatalog.cs:1 in 35e93ef. [](commit_id = 35e93ef, deletion_comment = True)

@artidoro
Copy link
Contributor Author

    public override SchemaShape GetOutputSchema(SchemaShape inputSchema)

Yes you are right let me do that.


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


Refers to: src/Microsoft.ML.Transforms/CustomMappingTransformer.cs:207 in 35e93ef. [](commit_id = 35e93ef, deletion_comment = False)

@artidoro
Copy link
Contributor Author

    public RandomModelParameters(IHostEnvironment env, int seed)

Ok will make this internal thank you!


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


Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:142 in 35e93ef. [](commit_id = 35e93ef, deletion_comment = False)

@artidoro
Copy link
Contributor Author

    public PriorModelParameters(IHostEnvironment env, float prob)

Thank you!


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


Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:379 in 35e93ef. [](commit_id = 35e93ef, deletion_comment = False)

Copy link
Member

@abgoswam abgoswam left a comment

Choose a reason for hiding this comment

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

:shipit:

@artidoro artidoro merged commit f8e58dd into dotnet:master Feb 13, 2019
@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #2510 into master will increase coverage by <.01%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##           master    #2510      +/-   ##
==========================================
+ Coverage   71.25%   71.25%   +<.01%     
==========================================
  Files         797      797              
  Lines      141271   141280       +9     
  Branches    16115    16115              
==========================================
+ Hits       100663   100671       +8     
- Misses      36148    36149       +1     
  Partials     4460     4460
Flag Coverage Δ
#Debug 71.25% <97.82%> (ø) ⬆️
#production 67.58% <97.43%> (ø) ⬆️
#test 85.36% <100%> (ø) ⬆️


namespace Microsoft.ML.Samples.Dynamic
{
public class RandomTrainerSample
Copy link
Contributor

Choose a reason for hiding this comment

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

RandomTrainerSample [](start = 17, length = 19)

Zeeshan A, Shahab and me have PRs where we create BinaryClassification folder.
Any chance you can move this two to that folder?

@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.

Creation of components through MLContext: advanced options and other feedback
3 participants