[SPARK-12424][ML] The implementation of ParamMap#filter is wrong. #10381
Conversation
| // Not using filterKeys also avoid SI-6654 | ||
| val filtered = map.filter { | ||
| case (k, _) => | ||
| k.parent == parent.uid |
There was a problem hiding this comment.
the indent seems to be broken
|
LGTM except one minor comment. |
|
Test build #48008 has finished for PR 10381 at commit
|
|
Test build #48010 has finished for PR 10381 at commit
|
| // returns the instance of collections.Map, not mutable.Map. | ||
| // Otherwise, we get ClassCastException. | ||
| // Not using filterKeys also avoid SI-6654 | ||
| val filtered = map.filter { |
There was a problem hiding this comment.
LGTM. This could be on one line but I wouldn't change it
|
Test build #48019 has finished for PR 10381 at commit
|
|
cc: @mengxr @jkbradley |
|
LGTM |
| // 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 } |
There was a problem hiding this comment.
Hasn't this changed the logic slightly? now you compare to parent.uid
There was a problem hiding this comment.
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)
|
Test build #48364 has finished for PR 10381 at commit
|
|
Thanks @srowen , @BenFradet and @yanboliang for the review. I'm merging this into |
ParamMap#filter uses `mutable.Map#filterKeys`. The return type of `filterKey` is collection.Map, not mutable.Map but the result is casted to mutable.Map using `asInstanceOf` so we get `ClassCastException`. Also, the return type of Map#filterKeys is not Serializable. It's the issue of Scala (https://issues.scala-lang.org/browse/SI-6654). Author: Kousuke Saruta <[email protected]> Closes #10381 from sarutak/SPARK-12424. (cherry picked from commit 07165ca) Signed-off-by: Kousuke Saruta <[email protected]>
ParamMap#filter uses
mutable.Map#filterKeys. The return type offilterKeyis collection.Map, not mutable.Map but the result is casted to mutable.Map usingasInstanceOfso we getClassCastException.Also, the return type of Map#filterKeys is not Serializable. It's the issue of Scala (https://issues.scala-lang.org/browse/SI-6654).