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

Make ScoreMapperSchema and its relatives not ISchema #2107

Merged
merged 3 commits into from Jan 11, 2019

Conversation

Projects
None yet
3 participants
@wschin
Copy link
Member

wschin commented Jan 10, 2019

  1. Remove ScoreMapperSchema and its relatives entirely
  2. Create static functions to generate commonly-used Schema

This PR wants to join the march of #1501.

@wschin wschin self-assigned this Jan 10, 2019

/// </summary>
public abstract class ScoreMapperSchemaBase : ISchema
[BestFriend]
internal static class ScoreSchemaFactory

This comment has been minimized.

@eerhardt

eerhardt Jan 10, 2019

Member

Can we rename this file to match the class that is inside of it? ScoreSchemaFactory.cs

This comment has been minimized.

@wschin

wschin Jan 10, 2019

Member

Sure.

}
}
// Schema of Score column. We are going to extend it by adding a Probability column.
var partialSchema = Create(NumberType.Float, MetadataUtils.Const.ScoreColumnKind.BinaryClassification, scoreColumnName);

This comment has been minimized.

@eerhardt

eerhardt Jan 10, 2019

Member

(minor) It feels a bit heavy-weight to create a whole schema object just to get 1 column out of it.

This comment has been minimized.

@wschin

wschin Jan 10, 2019

Member

This saves lines of creating metadata for that column (~6 lines). :)

@eerhardt
Copy link
Member

eerhardt left a comment

:shipit:

Make ScoreMapperSchema and its relatives not ISchema
1. Remove ScoreMapperSchema and its relatives entirely
2. Create static functions to generate commonly-used Schema

@wschin wschin force-pushed the wschin:make-score-mapper-schema-not-ischema branch from 813a0ff to acf718f Jan 10, 2019

}
}

public sealed class SequencePredictorSchema : ScoreMapperSchemaBase

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Jan 10, 2019

Member

SequencePredictorSchema [](start = 24, length = 23)

We use this schema in internal schema, if you remove it, that would force someone else to write code in internal repo.

This comment has been minimized.

@eerhardt

eerhardt Jan 10, 2019

Member

@Ivanidzo4ka - that's the intention. ISchema is going away, so any code (internal or public) that depends on it will need to be updated. #Resolved

This comment has been minimized.

@wschin

wschin Jan 10, 2019

Member

@ivmatan@microsoft.com, after running 5 out of 7 sequence-related TLC tests, I still don't find a place where this object is actually used. In TLC code base, SequencePredictorSchema's constructor always bypasses the part which distinguish it from ScoreMapperSchemaBase because keyNames.Length is always 0. Therefore, we don't even know if this class was created correctly. Is a replacement to this class required?

[Update] I am adding a test based on what I can find in TLC and creating a factory maethod to sequence predictor.


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

This comment has been minimized.

@wschin

wschin Jan 10, 2019

Member

Just added it back with tests. Please take a look.


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

@Ivanidzo4ka
Copy link
Member

Ivanidzo4ka left a comment

:shipit:

@wschin wschin merged commit d26510f into dotnet:master Jan 11, 2019

2 checks passed

MachineLearning-CI #20190110.43 succeeded
Details
license/cla All CLA requirements met.
Details

@wschin wschin deleted the wschin:make-score-mapper-schema-not-ischema branch Jan 11, 2019

@eerhardt

This comment has been minimized.

Copy link
Member

eerhardt commented Jan 11, 2019

using System;

Can you add a copyright header?


Refers to: test/Microsoft.ML.Core.Tests/UnitTests/ScoreSchemaTest.cs:1 in e1ad7d3. [](commit_id = e1ad7d3, deletion_comment = False)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment