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 SchemaShape.Column a struct instead of a class #1822

Merged
merged 13 commits into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@wschin
Member

wschin commented Dec 4, 2018

Fixes #1706.

@wschin wschin self-assigned this Dec 4, 2018

wschin added some commits Dec 4, 2018

@wschin wschin changed the title from [WIP] Make SchemaShape.Column a struct instead of a class to Make SchemaShape.Column a struct instead of a class Dec 4, 2018

@wschin wschin requested review from sfilipi and Ivanidzo4ka Dec 4, 2018

/// This documents that the parameter can not legally be null because it is struct.
/// </summary>
[Conditional("INVARIANT_CHECKS")]
public static void CheckValueOrDefault<T>(T val) where T : struct

This comment has been minimized.

@TomFinley

TomFinley Dec 4, 2018

Contributor

CheckValueOrDefault [](start = 27, length = 19)

Given that we're now doing this through other means, should probably revert these changes as well.

This comment has been minimized.

@wschin

wschin Dec 4, 2018

Member

Ok. Note that in TrainerEstimatorBase.cs we now do

        private protected TrainerEstimatorBase(IHost host,
            SchemaShape.Column feature,
            SchemaShape.Column label,
            SchemaShape.Column weight = default)
        {
            Contracts.CheckValue(host, nameof(host));
            Host = host;
            Host.Check(feature.IsValid, nameof(feature));
            Host.Check(label.IsValid, nameof(label));

            FeatureColumn = feature;
            LabelColumn = label;
            WeightColumn = weight;
        }

after removing CheckValueOrDefault(weight, nameof(weight)) and CheckValueOrDefault(label, nameof(label)).


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

@@ -367,7 +367,7 @@ public static bool IsNormalized(this Schema schema, int col)
/// of a scalar <see cref="BoolType"/> type, which we assume, if set, should be <c>true</c>.</returns>
public static bool IsNormalized(this SchemaShape.Column col)
{
Contracts.CheckValue(col, nameof(col));
Contracts.Check(col.IsValid, nameof(col));

This comment has been minimized.

@TomFinley

TomFinley Dec 4, 2018

Contributor

Check [](start = 22, length = 5)

This would be CheckParam. Also you will want to provide some sort of message. #Resolved

This comment has been minimized.

@wschin

wschin Dec 4, 2018

Member

Sure. We will use Contracts.CheckParam(col.IsValid, nameof(col), "struct not initialized"); I will do the same for other public functions.


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

This comment has been minimized.

@TomFinley

TomFinley Dec 5, 2018

Contributor

Thanks

@TomFinley

Just an urgent note about Schema.

@@ -16,13 +17,17 @@ namespace Microsoft.ML.Core.Data
/// This is more relaxed than the proper <see cref="ISchema"/>, since it's only a subset of the columns,
/// and also since it doesn't specify exact <see cref="ColumnType"/>'s for vectors and keys.
/// </summary>
public sealed class SchemaShape
public sealed class SchemaShape : IReadOnlyList<SchemaShape.Column>

This comment has been minimized.

@TomFinley

TomFinley Dec 5, 2018

Contributor

: IReadOnlyList<SchemaShape.Column> [](start = 36, length = 35)

This is not right! You can make this a ReadOnlyList with proper indexers and so forth. Or you could expose this Columns thing as you do below. You CANNOT DO BOTH.

This comment has been minimized.

@wschin

wschin Dec 5, 2018

Member

You're absolutely right. Thanks a lot for pointing this out.


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

@@ -163,7 +163,7 @@ protected override void CheckLabels(RoleMappedData data)
protected override void CheckLabelCompatible(SchemaShape.Column labelCol)
{
Contracts.AssertValue(labelCol);
Contracts.Assert(labelCol.IsValid, nameof(labelCol));

This comment has been minimized.

@TomFinley

TomFinley Dec 5, 2018

Contributor

, nameof(labelCol) [](start = 45, length = 18)

This is an assert. It doesn't need or benefit from a message. Also this message, being just the parameter name, might not be the most helpful. (Again not suggesting that it needs a message, just pointing out that if it needed one this would not necessarily be helpful. But please don't add one.)

This comment has been minimized.

@wschin

wschin Dec 5, 2018

Member

Fixed this and other places. Thanks.


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

@wschin wschin merged commit d7d4e99 into dotnet:master Dec 5, 2018

2 checks passed

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

@wschin wschin deleted the wschin:schemashape-column-api branch Dec 5, 2018

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