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.

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

Adding functional tests for all training and evaluation tasks #2646

Merged
merged 12 commits into from
Feb 24, 2019

Conversation

rogancarr
Copy link
Contributor

This PR adds functional tests for all training and evaluation tasks.

Fixes #2621

@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #2646 into master will increase coverage by 0.09%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master    #2646      +/-   ##
==========================================
+ Coverage   71.59%   71.69%   +0.09%     
==========================================
  Files         805      808       +3     
  Lines      142004   142102      +98     
  Branches    16119    16115       -4     
==========================================
+ Hits       101673   101879     +206     
+ Misses      35891    35788     -103     
+ Partials     4440     4435       -5
Flag Coverage Δ
#Debug 71.69% <96.29%> (+0.09%) ⬆️
#production 67.96% <ø> (+0.06%) ⬆️
#test 85.81% <96.29%> (+0.08%) ⬆️
Impacted Files Coverage Δ
...icrosoft.ML.Functional.Tests/Datasets/Sentiment.cs 0% <0%> (ø)
test/Microsoft.ML.Functional.Tests/Prediction.cs 100% <100%> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/Evaluation.cs 100% <100%> (ø)
test/Microsoft.ML.TestFramework/Datasets.cs 100% <100%> (ø) ⬆️
...est/Microsoft.ML.Functional.Tests/Datasets/Iris.cs 100% <100%> (ø)
test/Microsoft.ML.Functional.Tests/Validation.cs 100% <100%> (ø) ⬆️
...ional.Tests/Datasets/TrivialMatrixFactorization.cs 70% <70%> (ø)
...soft.ML.Functional.Tests/Datasets/MnistOneClass.cs 83.33% <83.33%> (ø)
test/Microsoft.ML.Functional.Tests/Common.cs 98.55% <94.73%> (-1.45%) ⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs 70.95% <0%> (-6.99%) ⬇️
... and 27 more

@artidoro artidoro self-requested a review February 21, 2019 20:45
// Train the model.
var model = pipeline.Fit(trainData);

// Evaulate the model.
Copy link
Contributor

@artidoro artidoro Feb 21, 2019

Choose a reason for hiding this comment

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

Evaulate [](start = 15, length = 8)

I think there is small typo that was copy pasted everywhere, can you fix that? #Resolved

/// </summary>
/// <remarks>
/// TODO #2644: At times, AnomalyDetection.Evaluate will return a set of NaN metrics.
/// </remarks>
Copy link
Contributor

@artidoro artidoro Feb 21, 2019

Choose a reason for hiding this comment

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

Maybe would not add the TODO to a xml comment. I would suggest having it as a regular comment only. #Resolved

/// Check that a <see cref="BinaryClassificationMetrics"/> object is valid.
/// </summary>
/// <param name="metrics">The metrics object.</param>
public static void CheckMetrics(BinaryClassificationMetrics metrics)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Feb 22, 2019

Choose a reason for hiding this comment

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

BinaryClassificationMetrics [](start = 40, length = 27)

we added CalibratedBinaryClassificationMetrics recently. Strangely you don't have them in your PR. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I hadn't noticed that.


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

Copy link
Contributor

@artidoro artidoro left a comment

Choose a reason for hiding this comment

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

Once you fix the build and add the check and related test with CalibratedBinaryClassificationMetrics it should be good to go.

@rogancarr
Copy link
Contributor Author

Once you fix the build and add the check and related test with CalibratedBinaryClassificationMetrics it should be good to go.

This is a weird build error. When the build systems do it, it's not handling the method overloading correctly. I can't repro it locally....

var metrics = mlContext.BinaryClassification.Evaluate(scoredData);

// Check that the metrics returned are valid.
Common.CheckMetrics(metrics);
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Feb 22, 2019

Choose a reason for hiding this comment

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

CheckMetrics [](start = 19, length = 12)

So the reason why I mentioned CalibratedBinaryClassificationMetrics is not only to add them to Common.cs, but also to have test on them.
Maybe have two pipelines in this test. One with calibrated, one without.
#Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha ;)


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

{
output.Label = input.Label;
// The standard set used in tests has 150 rows
output.GroupId = (ushort)rng.Next(0, 30);
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Feb 22, 2019

Choose a reason for hiding this comment

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

ushort [](start = 34, length = 6)

any reason why it's ushort? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remnants of when I was trying to read directly as a KeyType. (see #2642)


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

namespace Microsoft.ML.Functional.Tests.Datasets
{
/// <summary>
/// A class containing one property per <see cref="DataKind"/>.
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Feb 22, 2019

Choose a reason for hiding this comment

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

/// A class containing one property per . [](start = 4, length = 63)

I don't get this sentence. What DataKind has to do with MF? Your data is row+col+value triplets for matrix. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy/Paste error, unfortunately.


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

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@rogancarr rogancarr merged commit 8001ccc into dotnet:master Feb 24, 2019
@rogancarr rogancarr deleted the 2621_evaluation_scenarios branch February 24, 2019 23:11
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants