Skip to content

[SPARK-22883] ML test for StructuredStreaming: spark.ml.feature, I-M #20964

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
Closed
jkbradley wants to merge 4 commits into from
Closed

[SPARK-22883] ML test for StructuredStreaming: spark.ml.feature, I-M #20964

jkbradley wants to merge 4 commits into from

Conversation

@jkbradley
Copy link
Member

@jkbradley jkbradley commented Apr 2, 2018

What changes were proposed in this pull request?

Adds structured streaming tests using testTransformer for these suites:

  • IDF
  • Imputer
  • Interaction
  • MaxAbsScaler
  • MinHashLSH
  • MinMaxScaler
  • NGram

How was this patch tested?

It is a bunch of tests!

WeichenXu123
WeichenXu123 reviewed Apr 3, 2018
View reviewed changes
mllib/src/test/scala/org/apache/spark/ml/feature/ImputerSuite.scala Outdated
(10.0, 10.0),
(Double.NaN, 8.0),
(null, 8.0)
).toDF("value", "expected_mean_value")
Copy link
Contributor

@WeichenXu123 WeichenXu123 Apr 3, 2018

Choose a reason for hiding this comment

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

Why the "value" column use java.lang.Double type ?

Copy link
Member Author

@jkbradley jkbradley Apr 3, 2018

Choose a reason for hiding this comment

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

since it's nullable

mllib/src/test/scala/org/apache/spark/ml/feature/MaxAbsScalerSuite.scala Outdated
assert(vector1.equals(vector2), s"MaxAbsScaler ut error: $vector2 should be $vector1")
testTransformer[(Vector, Vector)](df, model, "expected", "scaled") {
case Row(vector1: Vector, vector2: Vector) =>
assert(vector1.equals(vector2), s"MaxAbsScaler error: $vector2 should be $vector1")
Copy link
Contributor

@WeichenXu123 WeichenXu123 Apr 3, 2018

Choose a reason for hiding this comment

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

vector1 === vector2
and the error message looks strange. ==> s"scaled value $vector2 do not equal expected value "$vector1" ?

mllib/src/test/scala/org/apache/spark/ml/feature/MinHashLSHSuite.scala Outdated
output.foreach {
case hashOutput: Vector => assert(hashOutput.size === 1)
}
}
Copy link
Contributor

@WeichenXu123 WeichenXu123 Apr 3, 2018

Choose a reason for hiding this comment

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

Why not have "expected" column" here to compare with ?

Copy link
Member Author

@jkbradley jkbradley Apr 3, 2018

Choose a reason for hiding this comment

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

I don't think that's necessary for testing that this works with structured streaming. (I can't see how streaming would mess up the correctness of the algorithm.)

mllib/src/test/scala/org/apache/spark/ml/feature/MinMaxScalerSuite.scala Outdated
model.transform(df).select("expected", "scaled").collect()
.foreach { case Row(vector1: Vector, vector2: Vector) =>
testTransformer[(Vector, Vector)](df, model, "expected", "scaled") {
case Row(vector1: Vector, vector2: Vector) =>
assert(vector1.equals(vector2), "Transformed vector is different with expected.")
Copy link
Contributor

@WeichenXu123 WeichenXu123 Apr 3, 2018

Choose a reason for hiding this comment

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

vector1 === vector2

Copy link
Member Author

@jkbradley jkbradley Apr 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

True, === is more standard (though both are fine)

mllib/src/test/scala/org/apache/spark/ml/feature/NGramSuite.scala Outdated
@@ -84,7 +84,7 @@ class NGramSuite extends MLTest with DefaultReadWriteTest {

def testNGram(t: NGram, dataFrame: DataFrame): Unit = {
testTransformer[(Seq[String], Seq[String])](dataFrame, t, "nGrams", "wantedNGrams") {
case Row(actualNGrams : Seq[String], wantedNGrams: Seq[String]) =>
case Row(actualNGrams : Seq[_], wantedNGrams: Seq[_]) =>
Copy link
Contributor

@WeichenXu123 WeichenXu123 Apr 3, 2018

Choose a reason for hiding this comment

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

Just curious, why change Seq[String] to Seq[_], is it for fixing some potential issue ?

Copy link
Member Author

@jkbradley jkbradley Apr 3, 2018

Choose a reason for hiding this comment

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

String is not actually checked because of erasure, so IntelliJ complained with a style warning before this change.

@SparkQA
Copy link

SparkQA commented Apr 3, 2018

Test build #88851 has finished for PR 20964 at commit 6dff94e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@WeichenXu123
Copy link
Contributor

WeichenXu123 commented Apr 10, 2018

LGTM. 👍

@jkbradley
Copy link
Member Author

jkbradley commented Apr 11, 2018

Thanks! I'll rerun tests since they are stale and merge after they pass.

@SparkQA
Copy link

SparkQA commented Apr 11, 2018

Test build #4151 has finished for PR 20964 at commit 6dff94e.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Member Author

jkbradley commented Apr 11, 2018

I rebased off of master because of the merge warning in the last tests.
I did not have to resolve any conflicts.
I'll merge this once tests pass.

@SparkQA
Copy link

SparkQA commented Apr 11, 2018

Test build #89174 has finished for PR 20964 at commit 592670c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Member Author

jkbradley commented Apr 11, 2018

Merging with master

jkbradley added a commit to jkbradley/spark that referenced this pull request Apr 11, 2018
@jkbradley
[SPARK-22883] ML test for StructuredStreaming: spark.ml.feature, I-M
a420943 
## What changes were proposed in this pull request?

Adds structured streaming tests using testTransformer for these suites:
* IDF
* Imputer
* Interaction
* MaxAbsScaler
* MinHashLSH
* MinMaxScaler
* NGram

## How was this patch tested?

It is a bunch of tests!

Author: Joseph K. Bradley <joseph@databricks.com>

Closes apache#20964 from jkbradley/SPARK-22883-part2.
@asfgit asfgit closed this in 75a1830 Apr 11, 2018
@jkbradley jkbradley mentioned this pull request Apr 11, 2018
Closed
@jkbradley jkbradley deleted the SPARK-22883-part2 branch April 11, 2018 17:06
asfgit pushed a commit that referenced this pull request Apr 11, 2018
@jkbradley
[SPARK-22883][ML] ML test for StructuredStreaming: spark.ml.feature, I-M
acfc156 
This backports #20964 to branch-2.3.

## What changes were proposed in this pull request?

Adds structured streaming tests using testTransformer for these suites:
* IDF
* Imputer
* Interaction
* MaxAbsScaler
* MinHashLSH
* MinMaxScaler
* NGram

## How was this patch tested?

It is a bunch of tests!

Author: Joseph K. Bradley <josephdatabricks.com>

Author: Joseph K. Bradley <joseph@databricks.com>

Closes #21042 from jkbradley/SPARK-22883-part2-2.3backport.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WeichenXu123 WeichenXu123 WeichenXu123 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jkbradley @SparkQA @WeichenXu123