Skip to content

Change ML.NET to work with .NET Framework 4.6.1 #1075

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

Merged
yaeldekel merged 2 commits into from
Sep 28, 2018
Merged

Change ML.NET to work with .NET Framework 4.6.1 #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b2827a
Change all uses of System.Linq.Append to System.Linq.Concat.
yaeldMS Sep 27, 2018
4fb0ef0
Address code review comments
yaeldMS Sep 27, 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
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Data/DataLoadSave/EstimatorChain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.AppendElement(estimator), _scopes.AppendElement(scope));
}
}
}
9 changes: 9 additions & 0 deletions src/Microsoft.ML.Data/DataLoadSave/EstimatorExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using Microsoft.ML.Core.Data;
using Microsoft.ML.Runtime.Internal.Utilities;

namespace Microsoft.ML.Runtime.Data
{
Expand Down Expand Up @@ -123,5 +124,13 @@ public static IEstimator<TTransformer> WithOnFitDelegate<TTransformer>(this IEst
Contracts.CheckValue(onFit, nameof(onFit));
return new DelegateEstimator<TTransformer>(estimator, onFit);
}

internal static T[] AppendElement<T>(this T[] array, T element)
{
T[] result = new T[Utils.Size(array) + 1];
Array.Copy(array, result, result.Length - 1);
result[result.Length - 1] = element;
return result;
}
}
}
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

@ericstj ericstj Sep 28, 2018

Choose a reason for hiding this comment

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

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

}

public void Save(ModelSaveContext ctx)
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ML.Data/StaticPipe/StaticPipeInternalUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Runtime.CompilerServices;
using Microsoft.ML.Core.Data;
using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.Data;
using Microsoft.ML.Runtime.Internal.Utilities;

namespace Microsoft.ML.StaticPipe.Runtime
Expand Down Expand Up @@ -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)));
Copy link
Member

@ericstj ericstj Sep 28, 2018

Choose a reason for hiding this comment

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

It feels wrong to allocate an array and immediate throw it away to allocate one 1 element longer.

return methods;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

@eerhardt eerhardt Sep 27, 2018

Choose a reason for hiding this comment

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

Which order.Prepend method is this?

Copy link
Member

@eerhardt eerhardt Sep 27, 2018

Choose a reason for hiding this comment

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

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);
        }

Copy link
Author

@yaeldekel yaeldekel Sep 27, 2018

Choose a reason for hiding this comment

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

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)

}

public void SaveText(TextWriter writer, LinearBinaryPredictor parent, RoleMappedSchema schema, int paramCountCap)
Expand Down