Change ML.NET to work with .NET Framework 4.6.1 #1075
Conversation
| @@ -72,7 +72,7 @@ public EstimatorChain<TNewTrans> Append<TNewTrans>(IEstimator<TNewTrans> estimat | |||
| where TNewTrans : class, ITransformer | |||
| { | |||
| Contracts.CheckValue(estimator, nameof(estimator)); | |||
| return new EstimatorChain<TNewTrans>(_estimators.Append(estimator).ToArray(), _scopes.Append(scope).ToArray()); | |||
| return new EstimatorChain<TNewTrans>(_estimators.Concat(new[] { estimator }).ToArray(), _scopes.Concat(new[] { scope }).ToArray()); | |||
There was a problem hiding this comment.
I'm not sure how concerned we are about perf in these methods, but another way would be to make our own AppendElement method:
public static T[] AppendElement(T[] array, T element)
{
T[] result = new T[array.Length + 1];
Array.Copy(array, result);
result[array.Length] = element;
return result;
}Another option would be to use ImmutableCollections.... I don't think you'd have to pay a penalty for copying in that case. But I'm not sure we'd want to take a new dependency, if we aren't using ImmutableCollections already.
There was a problem hiding this comment.
Hi @eerhardt -- we are using ImmutableCollections, and I am beginning to use it in my code. I really like it a lot, I think we ought to use it more.
| @@ -342,7 +342,7 @@ public CoefficientStatistics[] GetCoefficientStatistics(LinearBinaryPredictor pa | |||
| return null; | |||
|
|
|||
| var order = GetUnorderedCoefficientStatistics(parent, schema).OrderByDescending(stat => stat.ZScore).Take(paramCountCap - 1); | |||
| return order.Prepend(new CoefficientStatistics("(Bias)", bias, stdError, zScore, pValue)).ToArray(); | |||
| return order.Prepend(new[] { new CoefficientStatistics("(Bias)", bias, stdError, zScore, pValue) }).ToArray(); | |||
There was a problem hiding this comment.
Which order.Prepend method is this?
There was a problem hiding this comment.
Ah, we have our own shim here: src\Microsoft.ML.Core\Data\MetadataUtils.cs
/// <summary>
/// Prepends a params array to an enumerable. Useful when implementing GetMetadataTypes.
/// </summary>
public static IEnumerable<T> Prepend<T>(this IEnumerable<T> tail, params T[] head)
{
return head.Concat(tail);
}
There was a problem hiding this comment.
We have an overload of this method that takes a params T[], so this still works with older versions of .NET. I thought it would be better to call the same overload independent of the framework, so I changed it.
In reply to: 221026867 [](ancestors = 221026867)
| @@ -342,7 +342,7 @@ public CoefficientStatistics[] GetCoefficientStatistics(LinearBinaryPredictor pa | |||
| return null; | |||
|
|
|||
| var order = GetUnorderedCoefficientStatistics(parent, schema).OrderByDescending(stat => stat.ZScore).Take(paramCountCap - 1); | |||
| return order.Prepend(new CoefficientStatistics("(Bias)", bias, stdError, zScore, pValue)).ToArray(); | |||
| return order.Prepend(new[] { new CoefficientStatistics("(Bias)", bias, stdError, zScore, pValue) }).ToArray(); | |||
There was a problem hiding this comment.
Ah, we have our own shim here: src\Microsoft.ML.Core\Data\MetadataUtils.cs
/// <summary>
/// Prepends a params array to an enumerable. Useful when implementing GetMetadataTypes.
/// </summary>
public static IEnumerable<T> Prepend<T>(this IEnumerable<T> tail, params T[] head)
{
return head.Concat(tail);
}
|
Closing/opening to re-check CI tests |
| @@ -75,7 +76,7 @@ private static MethodInfo[] InitValueTupleCreateMethods() | |||
| var methods = typeof(ValueTuple).GetMethods() | |||
| .Where(m => m.Name == methodName && m.ContainsGenericParameters) | |||
| .OrderBy(m => m.GetGenericArguments().Length).Take(7) | |||
| .Append(typeof(AnalyzeUtil).GetMethod(nameof(UnstructedCreate))).ToArray(); | |||
| .ToArray().AppendElement(typeof(AnalyzeUtil).GetMethod(nameof(UnstructedCreate))); | |||
There was a problem hiding this comment.
It feels wrong to allocate an array and immediate throw it away to allocate one 1 element longer.
| @@ -146,7 +146,7 @@ public TransformerChain<TNewLast> Append<TNewLast>(TNewLast transformer, Transfo | |||
| where TNewLast : class, ITransformer | |||
| { | |||
| Contracts.CheckValue(transformer, nameof(transformer)); | |||
| return new TransformerChain<TNewLast>(_transformers.Append(transformer).ToArray(), _scopes.Append(scope).ToArray()); | |||
| return new TransformerChain<TNewLast>(_transformers.AppendElement(transformer), _scopes.AppendElement(scope)); | |||
There was a problem hiding this comment.
Looks like this type only has enumerable constructor, so you end up with 3 arrays for each param. One that is size N, and two that are size N+1. If you keep it as an enumerable, you can reduce this. It can be further reduced if you have a constructor that takes an array with reference semantics.
It's also somewhat odd that TransformerChain has copy semantics for constructor params, but EstimatorChain has reference semantics (and thus doesn't have this array copy problem).
ghost
locked as resolved and limited conversation to collaborators
Fixes #1072 .