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

[SPARK-22799][ML] Bucketizer should throw exception if single- and multi-column params are both set #19993

Closed
wants to merge 19 commits into from
+131 −45
Closed

[SPARK-22799][ML] Bucketizer should throw exception if single- and multi-column params are both set #19993

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8f3581c
[SPARK-22799][ML] Bucketizer should throw exception if single- and mu…
mgaido91 Dec 15, 2017
bb0c0d2
address comments
mgaido91 Dec 19, 2017
9f56800
fix doc
mgaido91 Dec 19, 2017
f593f5b
fix mima error
mgaido91 Dec 19, 2017
2ecdc73
use ParamValidators
mgaido91 Dec 19, 2017
26fe05e
fix ut
mgaido91 Dec 19, 2017
64634b5
address review comments
mgaido91 Dec 19, 2017
9872bfd
add checkMultiColumnParams
mgaido91 Dec 20, 2017
d0b8d06
address review comments
mgaido91 Dec 21, 2017
b20fb91
remove two unneeded checks
mgaido91 Dec 21, 2017
09d652d
address comments
mgaido91 Dec 30, 2017
a0c0fed
remove isBucketizeMultipleColumns
mgaido91 Jan 8, 2018
25b9bd4
address comments
mgaido91 Jan 17, 2018
18bbf61
strengthened requirements about exclusive Params for single and multi…
jkbradley Jan 20, 2018
d9d25b0
Merge pull request #1 from jkbradley/mgaido91-SPARK-22799
mgaido91 Jan 20, 2018
8c162a3
fix style error
mgaido91 Jan 21, 2018
7894609
fixt ut error
mgaido91 Jan 21, 2018
ebc6d16
add all cases to UT
mgaido91 Jan 23, 2018
2bc5cb4
review comment
mgaido91 Jan 24, 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
8 changes: 4 additions & 4 deletions mllib/src/main/scala/org/apache/spark/ml/feature/Bucketizer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ final class Bucketizer @Since("1.4.0") (@Since("1.4.0") override val uid: String
* by `inputCol`. A warning will be printed if both are set.
Copy link
Member

Choose a reason for hiding this comment

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

We should modify the document above too.

*/
private[feature] def isBucketizeMultipleColumns(): Boolean = {
if (isSet(inputCols) && isSet(inputCol)) {
logWarning("Both `inputCol` and `inputCols` are set, we ignore `inputCols` and this " +
"`Bucketizer` only map one column specified by `inputCol`")
false
if (isSet(inputCols) && isSet(inputCol) || isSet(inputCols) && isSet(outputCol) ||
isSet(inputCol) && isSet(outputCols)) {
throw new IllegalArgumentException("Both `inputCol` and `inputCols` are set, `Bucketizer` " +
"only supports setting either `inputCol` or `inputCols`.")
Copy link
Contributor

@WeichenXu123 WeichenXu123 Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Here it is better to add one more check:
if single column, only set splits param.
if multiple column, only set splitsArray param. and check:
inputCols.length == outputCols.length == splitsArray.length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. I will add these checks while doing the changes requested by @hhbyyh. Thanks.

} else if (isSet(inputCols)) {
true
} else {
Expand Down
25 changes: 22 additions & 3 deletions mllib/src/test/scala/org/apache/spark/ml/feature/BucketizerSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,14 +402,33 @@ class BucketizerSuite extends SparkFunSuite with MLlibTestSparkContext with Defa
}

test("Both inputCol and inputCols are set") {
val bucket = new Bucketizer()
val feature1 = Array(-0.5, -0.3, 0.0, 0.2)
val feature2 = Array(-0.3, -0.2, 0.5, 0.0)
val df = feature1.zip(feature2).toSeq.toDF("feature1", "feature2")

val invalid1 = new Bucketizer()
.setInputCol("feature1")
.setOutputCol("result")
.setSplits(Array(-0.5, 0.0, 0.5))
.setInputCols(Array("feature1", "feature2"))

// When both are set, we ignore `inputCols` and just map the column specified by `inputCol`.
assert(bucket.isBucketizeMultipleColumns() == false)
val invalid2 = new Bucketizer()
.setOutputCol("result")
.setSplits(Array(-0.5, 0.0, 0.5))
.setInputCols(Array("feature1", "feature2"))

val invalid3 = new Bucketizer()
.setInputCol("feature1")
.setSplits(Array(-0.5, 0.0, 0.5))
.setOutputCols(Array("result1", "result2"))

Seq(invalid1, invalid2, invalid3).foreach { bucketizer =>
// When both inputCol and inputCols are set, we throw Exception.
val e = intercept[IllegalArgumentException] {
bucketizer.transform(df)
}
assert(e.getMessage.contains("Both `inputCol` and `inputCols` are set"))
}
}
}

Expand Down