Skip to content

[SPARK-12424][ML] The implementation of ParamMap#filter is wrong. #10381

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

Closed
sarutak wants to merge 4 commits into from
Closed

[SPARK-12424][ML] The implementation of ParamMap#filter is wrong. #10381

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95e3ff2
Fixed ParamMap#filter to avoid ClassCastException and SI-6654
sarutak Dec 18, 2015
d3fc82d
Fixed indentation
sarutak Dec 18, 2015
080e983
Modified test
sarutak Dec 18, 2015
ea924e9
Simplified test
sarutak Dec 28, 2015
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: 6 additions & 2 deletions mllib/src/main/scala/org/apache/spark/ml/param/params.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,8 +859,12 @@ final class ParamMap private[ml] (private val map: mutable.Map[Param[Any], Any])
* Filters this param map for the given parent.
*/
def filter(parent: Params): ParamMap = {
val filtered = map.filterKeys(_.parent == parent)
new ParamMap(filtered.asInstanceOf[mutable.Map[Param[Any], Any]])
// Don't use filterKeys because mutable.Map#filterKeys
// returns the instance of collections.Map, not mutable.Map.
// Otherwise, we get ClassCastException.
// Not using filterKeys also avoid SI-6654
val filtered = map.filter { case (k, _) => k.parent == parent.uid }
Copy link
Member

@srowen srowen Dec 24, 2015

Choose a reason for hiding this comment

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

Hasn't this changed the logic slightly? now you compare to parent.uid

Copy link
Member Author

@sarutak sarutak Dec 24, 2015

Choose a reason for hiding this comment

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

I think the original logic is wrong because the type of .parent is String (this is a member of Param) while the type of parameter parent is Params.

According to the implementation of Param, the member parent of Param is passed uid of Identifiable which is a trait of Params.

class Param[T](val parent: String, val name: String, val doc: String, val isValid: T => Boolean)
  extends Serializable {

  def this(parent: Identifiable, name: String, doc: String, isValid: T => Boolean) =
    this(parent.uid, name, doc, isValid)

new ParamMap(filtered)
}

/**
Expand Down
38 changes: 38 additions & 0 deletions mllib/src/test/scala/org/apache/spark/ml/param/ParamsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@

package org.apache.spark.ml.param

import java.io.{ByteArrayOutputStream, NotSerializableException, ObjectOutputStream}

import org.apache.spark.SparkFunSuite
import org.apache.spark.ml.util.MyParams
import org.apache.spark.mllib.linalg.{Vector, Vectors}

class ParamsSuite extends SparkFunSuite {
Expand Down Expand Up @@ -349,6 +352,41 @@ class ParamsSuite extends SparkFunSuite {
val t3 = t.copy(ParamMap(t.maxIter -> 20))
assert(t3.isSet(t3.maxIter))
}

test("Filtering ParamMap") {
val params1 = new MyParams("my_params1")
val params2 = new MyParams("my_params2")
val paramMap = ParamMap(
params1.intParam -> 1,
params2.intParam -> 1,
params1.doubleParam -> 0.2,
params2.doubleParam -> 0.2)
val filteredParamMap = paramMap.filter(params1)

assert(filteredParamMap.size === 2)
filteredParamMap.toSeq.foreach {
case ParamPair(p, _) =>
assert(p.parent === params1.uid)
}

// At the previous implementation of ParamMap#filter,
// mutable.Map#filterKeys was used internally but
// the return type of the method is not serializable (see SI-6654).
// Now mutable.Map#filter is used instead of filterKeys and the return type is serializable.
// So let's ensure serializability.
val objOut = new ObjectOutputStream(new ByteArrayOutputStream())
try {
objOut.writeObject(filteredParamMap)
} catch {
case _: NotSerializableException =>
fail("The field of ParamMap 'map' may not be serializable. " +
"See SI-6654 and the implementation of ParamMap#filter")
case e: Exception =>
Copy link
Member

@srowen srowen Dec 28, 2015

Choose a reason for hiding this comment

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

This isn't necessary; an exception fails the test already. Really, same for the not-serializable exception. You don't need to close a stream that is just a byte-array output stream. Still it's all fine. Merge whenever you like.

Copy link
Member Author

@sarutak sarutak Dec 28, 2015

Choose a reason for hiding this comment

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

Oh, yes, ByteArrayOutputStream doesn't need to be closed. I'll simplify it.

fail(s"Exception was thrown unexpectedly during the serializability test: ${e.getMessage}")
} finally {
objOut.close()
}
}
}

object ParamsSuite extends SparkFunSuite {
Expand Down