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.

Sign up for GitHub

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

Jump to bottom

Use HideEnumValueAttribute for both manifest and C# API generation. #356

Merged
merged 2 commits into from
Jun 18, 2018
Merged

Use HideEnumValueAttribute for both manifest and C# API generation. #356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bc6dad
Use HideEnumValueAttribute for both manifest and C# API generation.
Jun 13, 2018
7f94dd0
Unhide NAReplaceTransform.ReplacementKind.SpecifiedValue.
Jun 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 4 additions & 15 deletions src/Microsoft.ML/CSharpApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11058,14 +11058,10 @@ namespace Transforms
{
public enum NAHandleTransformReplacementKind
{
Default = 0,
Def = 0,
DefaultValue = 0,
Mean = 1,
Minimum = 2,
Min = 2,
Maximum = 3,
Max = 3
Maximum = 3
}


Expand Down Expand Up @@ -11153,7 +11149,7 @@ public void AddColumn(string outputColumn, string inputColumn)
/// <summary>
/// The replacement method to utilize
/// </summary>
public NAHandleTransformReplacementKind ReplaceWith { get; set; } = NAHandleTransformReplacementKind.Def;
public NAHandleTransformReplacementKind ReplaceWith { get; set; } = NAHandleTransformReplacementKind.DefaultValue;

/// <summary>
/// Whether to impute values by slot
Expand Down Expand Up @@ -11527,17 +11523,10 @@ namespace Transforms
{
public enum NAReplaceTransformReplacementKind
{
Default = 0,
DefaultValue = 0,
Def = 0,
Mean = 1,
Min = 2,
Minimum = 2,
Max = 3,
Maximum = 3,
SpecifiedValue = 4,
Val = 4,
Value = 4
Maximum = 3
}


Expand Down Expand Up @@ -11625,7 +11614,7 @@ public void AddColumn(string outputColumn, string inputColumn)
/// <summary>
/// The replacement method to utilize
/// </summary>
public NAReplaceTransformReplacementKind ReplacementKind { get; set; } = NAReplaceTransformReplacementKind.Def;
public NAReplaceTransformReplacementKind ReplacementKind { get; set; } = NAReplaceTransformReplacementKind.DefaultValue;

/// <summary>
/// Whether to impute values by slot
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ML/Runtime/EntryPoints/JsonUtils/JsonManifestUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ private static JToken BuildTypeToken(IExceptionContext ectx, FieldInfo fieldInfo
case TlcModule.DataKind.Enum:
jo = new JObject();
jo[FieldNames.Kind] = typeEnum.ToString();
var values = Enum.GetNames(type);
var values = Enum.GetNames(type).Where(n => type.GetField(n).GetCustomAttribute<HideEnumValueAttribute>() == null);
jo[FieldNames.Values] = new JArray(values);
return jo;
case TlcModule.DataKind.Array:
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.ML/Runtime/Internal/Tools/CSharpApiGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Reflection;
using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.CommandLine;
using Microsoft.ML.Runtime.Data;
Expand Down Expand Up @@ -156,6 +157,8 @@ private void GenerateEnums(IndentingTextWriter writer, Type inputType, string cu
for (int i = 0; i < names.Length; i++)
{
var name = names[i];
if (type.GetField(name).GetCustomAttribute<HideEnumValueAttribute>() != null)
continue;
var value = values.GetValue(i);
writer.WriteLine(prefix);
if (enumType == typeof(int))
Expand Down
17 changes: 15 additions & 2 deletions src/Microsoft.ML/Runtime/Internal/Tools/CSharpGeneratorUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.CodeDom;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using Microsoft.CSharp;
using Microsoft.ML.Runtime.CommandLine;
using Microsoft.ML.Runtime.EntryPoints;
Expand Down Expand Up @@ -279,10 +280,22 @@ public static string GetValue(ModuleCatalog catalog, Type fieldType, object fiel
case TlcModule.DataKind.Bool:
return (bool)fieldValue ? "true" : "false";
case TlcModule.DataKind.Enum:
string enumAsString = fieldValue.ToString();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is something of an unfortunate engineering situation that the so-called C# API code generator and the manifest JSON generator utilize, as near as I can see, entirely separate codepaths to interact with the same structures.

if (fieldType.GetField(enumAsString).GetCustomAttribute<HideEnumValueAttribute>() != null)
{
// The default value for the enum has the hiding attribute on it. We will search for
// alternate names. Regrettably I see no way beyond a manual scan.

string unhiddenName = Enum.GetNames(fieldType).Zip(Enum.GetValues(fieldType).Cast<object>(), (name, val) => (name, val))
.Where(pair => pair.val.Equals(fieldValue))
.Where(pair => fieldType.GetField(pair.name).GetCustomAttribute<HideEnumValueAttribute>() == null)
.Select(pair => pair.name).FirstOrDefault();
enumAsString = unhiddenName ?? throw Contracts.Except($"Could not find unhidden alternative for '{fieldValue}' in type '{fieldType}'");
}
if (generatedClasses.IsGenerated(fieldType.FullName))
return generatedClasses.GetApiName(fieldType, rootNameSpace) + "." + fieldValue;
return generatedClasses.GetApiName(fieldType, rootNameSpace) + "." + enumAsString;
else
return generatedClasses.GetApiName(fieldType, "Runtime") + "." + fieldValue;
return generatedClasses.GetApiName(fieldType, "Runtime") + "." + enumAsString;
case TlcModule.DataKind.Char:
return $"'{GetCharAsString((char)fieldValue)}'";
case TlcModule.DataKind.Component:
Expand Down
30 changes: 4 additions & 26 deletions test/BaselineOutput/Common/EntryPoints/core_manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15390,14 +15390,10 @@
"Type": {
"Kind": "Enum",
"Values": [
"Default",
"Def",
"DefaultValue",
"Mean",
"Minimum",
"Min",
"Maximum",
"Max"
"Maximum"
]
},
"Desc": "The replacement method to utilize",
Expand Down Expand Up @@ -15478,14 +15474,10 @@
"Type": {
"Kind": "Enum",
"Values": [
"Default",
"Def",
"DefaultValue",
"Mean",
"Minimum",
"Min",
"Maximum",
"Max"
"Maximum"
]
},
"Desc": "The replacement method to utilize",
Expand Down Expand Up @@ -15780,17 +15772,10 @@
"Type": {
"Kind": "Enum",
"Values": [
"Default",
"DefaultValue",
"Def",
"Mean",
"Min",
"Minimum",
"Max",
"Maximum",
"SpecifiedValue",
"Val",
"Value"
Copy link
Member

Choose a reason for hiding this comment

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

All three of them are marked as hidden. Do you know if that is intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sfilipi thanks for the question. Hiding the "abbreviated" versions Val, Value, I think is probably intentional. SpecifiedValue though is an interesting choice. I am not certain whether hiding all options related to this is intentional, though assuredly it was done.

machinelearning/src/Microsoft.ML.Transforms/NAReplaceTransform.cs

Lines 56 to 57 in c023727

[HideEnumValue]
SpecifiedValue,

@yaeldekel wrote this enum, but that was back in 2016. She may not remember, but just tagging her on the off chance she does.

Copy link
Member

@sfilipi sfilipi Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should probably expose the "SpecifiedValue". Checking the NA Replace transform, if the enum is not set to that, the value set for the ReplacementString is ignored.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sfilipi I agree with that logic. I will unhide.

"Maximum"
]
},
"Desc": "The replacement method to utilize",
Expand Down Expand Up @@ -15856,17 +15841,10 @@
"Type": {
"Kind": "Enum",
"Values": [
"Default",
"DefaultValue",
"Def",
"Mean",
"Min",
"Minimum",
"Max",
"Maximum",
"SpecifiedValue",
"Val",
"Value"
Copy link
Member

Choose a reason for hiding this comment

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

same check as above, about this argument being hidden.

"Maximum"
]
},
"Desc": "The replacement method to utilize",
Expand Down