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

Ensure correct metrics despite model failures on some CV folds #404

Merged
merged 17 commits into from
Sep 11, 2019
Merged

Ensure correct metrics despite model failures on some CV folds #404

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ca2afde
allow for model to fail on some CV folds and still get the metrics ca…
leahmcguire Sep 9, 2019
cf373e8
fixed import
leahmcguire Sep 9, 2019
e5b76c0
changed strategy to throw out partial failures
leahmcguire Sep 9, 2019
d9f07eb
got rid of _._1 notation and fixed test that was failing because of w…
leahmcguire Sep 9, 2019
5f92294
changed to flatmap and sumbykey
leahmcguire Sep 10, 2019
7f40ad2
removing map
leahmcguire Sep 10, 2019
8fe125e
remove toSeq
leahmcguire Sep 10, 2019
7f8eb4b
removed require
leahmcguire Sep 10, 2019
85eeab2
scalastyle fix
gerashegalov Sep 10, 2019
917f13c
cleanup
leahmcguire Sep 10, 2019
872fe35
Merge branch 'lm/cvFix' of github.com:salesforce/TransmogrifAI into l…
leahmcguire Sep 10, 2019
2714cd5
explicity added summation rules
leahmcguire Sep 10, 2019
842d016
test fix
leahmcguire Sep 10, 2019
39189a2
test fix
leahmcguire Sep 10, 2019
71649f1
test fix
leahmcguire Sep 10, 2019
17cb9fa
test fix
leahmcguire Sep 10, 2019
6687a34
cleanup
leahmcguire Sep 10, 2019
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
25 changes: 16 additions & 9 deletions core/src/main/scala/com/salesforce/op/stages/impl/tuning/OpCrossValidation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,23 @@ private[op] class OpCrossValidation[M <: Model[_], E <: Estimator[_]]
private def findBestModel(
folds: Seq[ValidatedModel[E]]
): ValidatedModel[E] = {
val metrics = folds.map(_.metrics).reduce(_ + _)
blas.dscal(metrics.length, 1.0 / numFolds, metrics, 1)
val ValidatedModel(est, _, _, grid) = folds.head
log.info(s"Average cross-validation for $est metrics: {}", metrics.toSeq.mkString(","))
val (bestMetric, bestIndex) =
if (evaluator.isLargerBetter) metrics.zipWithIndex.maxBy(_._1)
else metrics.zipWithIndex.minBy(_._1)
log.info(s"Best set of parameters:\n${grid(bestIndex)}")
require(folds.map(_.model.uid).toSet.size == 1) // Should be called only on instances of the same model
Copy link
Contributor

Choose a reason for hiding this comment

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

good to be defensive, my concern we call it in iteration in a private method completely in the scope here, and by construction we already know that folds are for the same model.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I can remove and just put a description on the method

val gridCounts = folds.map(_.grids.map(_ -> 1).toMap).reduce(_ + _)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it the same as

folds.flatMap(_.grids.map(_ -> 1)).sumByKey

val maxFolds = gridCounts.maxBy(_._2)._2
Copy link
Collaborator

Choose a reason for hiding this comment

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

For better readability please replace _._2 in val maxFolds = gridCounts.maxBy(_._2)._2 with val (_, maxFolds) = gridCounts.maxBy { case (_, folds) => folds }
Same in a few operations below.

val gridsIn = gridCounts.filter(_._2 == maxFolds).keySet
val gridMetrics = folds.map(f => f.grids.zip(f.metrics).toMap).reduce(_ + _)
.filterKeys(gridsIn.contains)
Copy link
Contributor

Choose a reason for hiding this comment

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

let us filter first, so we have less to reduce, maybe

    val gridMetrics = folds.flatMap(f => f.grids.zip(f.metrics))
      .collect { case (pm, met) if gridsIn.contains(pm) => (pm, met / maxFolds) }
      .sumByKey

.map{ case (key, met) => key -> met / maxFolds}
.toSeq
val ((bestGrid, bestMetric), bestIndex) =
if (evaluator.isLargerBetter) gridMetrics.zipWithIndex.maxBy(_._1._2)
else gridMetrics.zipWithIndex.minBy(_._1._2)
val ValidatedModel(est, _, _, _) = folds.head
log.info(s"Average cross-validation for $est metrics: {}", gridMetrics.mkString(","))
log.info(s"Best set of parameters:\n$bestGrid")
log.info(s"Best cross-validation metric: $bestMetric.")
ValidatedModel(est, bestIndex, metrics, grid)
val (grid, metrics) = gridMetrics.unzip
ValidatedModel(est, bestIndex, metrics.toArray, grid.toArray)
}

private[op] override def validate[T](
Expand Down
27 changes: 25 additions & 2 deletions ...src/test/scala/com/salesforce/op/stages/impl/regression/RegressionModelSelectorTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ class RegressionModelSelectorTest extends FlatSpec with TestSparkContext
justScores.length shouldEqual transformedData.count()
}

it should "fit and predict for even when some models fail" in {
it should "fit and predict even when some models fail" in {
val testEstimator = RegressionModelSelector
.withCrossValidation(
numFolds = 4,
Expand All @@ -240,11 +240,34 @@ class RegressionModelSelectorTest extends FlatSpec with TestSparkContext
assert(metaData.trainEvaluation.toJson(false).contains(s"${metric.entryName}"),
s"Metric ${metric.entryName} is not present in metadata: " + metaData)
)
metaData.validationResults.foreach(println(_))
metaData.validationResults.size shouldBe 42
}


it should "fit and predict even when some parameter settings fail for one of the models" in {
val testEstimator = RegressionModelSelector
.withCrossValidation(
numFolds = 4,
validationMetric = Evaluators.Regression.mse(),
seed = 10L,
modelTypesToUse = Seq(RMT.OpGeneralizedLinearRegression)
)
.setInput(label, features)


val model = testEstimator.fit(data)
model.evaluateModel(data)

// evaluation metrics from train set should be in metadata
val metaData = ModelSelectorSummary.fromMetadata(model.getMetadata().getSummaryMetadata())
RegressionEvalMetrics.values.foreach(metric =>
assert(metaData.trainEvaluation.toJson(false).contains(s"${metric.entryName}"),
s"Metric ${metric.entryName} is not present in metadata: " + metaData)
)
metaData.validationResults.size shouldBe 32
}


it should "fail when all models fail due to inappropriate data" in {

val glr = new OpGeneralizedLinearRegression()
Expand Down