[SPARK-15605] [ML] [Examples] Fix broken ML JavaDeveloperApiExample. #13353
Conversation
|
Test build #59478 has finished for PR 13353 at commit
|
|
I don't think the answer is to remove it; the example used to work. We should identify which PR broke it and try to fix it. I'd guess it's some change to Params or another abstraction. |
yanboliang
changed the title
|
Test build #59736 has finished for PR 13353 at commit
|
|
@jkbradley Yes, I have found another way to fix this issue. Please let me know whether this fix is ok to you. |
| * Param for max number of iterations | ||
| * <p> | ||
| * NOTE: The usual way to add a parameter to a model or algorithm is to include: | ||
| * - val myParamName: ParamType |
There was a problem hiding this comment.
This is kinda confusing for a Java example having Scala code, no?
There was a problem hiding this comment.
As discussed with @jkbradley , we will remove this example for 2.0. I will address this comments when we add this example back.
|
Thanks for exploring this! I had hoped we could figure out how to fix the abstractions. However, if it is not feasible in a short time, then maybe we should remove the example and add it back in later once we can fix the abstractions. I'll spend some more time trying to track down the change in the abstractions which broke things. Update: I guess it's that What do you think? |
|
@jkbradley I found the main problem is that the |
|
@jkbradley @MLnick I removed this example now and it's ready to be get in. I think we can left the JIRA open and target to 2.1. Please feel free to assign it to me for further work, I have some ideas in my mind. |
|
Test build #59844 has finished for PR 13353 at commit
|
|
LGTM @yanboliang do you want to try committing this? |
|
@srowen Please help to merge it. I have not set up commit environment completely and I will try to get it work in a few days. Thanks! |
|
Merged to master/2.0 but the JIRA is not closed |
## What changes were proposed in this pull request? See [SPARK-15605](https://issues.apache.org/jira/browse/SPARK-15605) for the detail of this bug. This PR fix 2 major bugs in this example: * The java example class use Param ```maxIter```, it will fail when calling ```Param.shouldOwn```. We need add a public method which return the ```maxIter``` Object. Because ```Params.params``` use java reflection to list all public method whose return type is ```Param```, and invoke them to get all defined param objects in the instance. * The ```uid``` member defined in Java class will be initialized after Scala traits such as ```HasFeaturesCol```. So when ```HasFeaturesCol``` being constructed, they get ```uid``` with null, which will cause ```Param.shouldOwn``` check fail. so, here is my changes: * Add public method: ```public IntParam getMaxIterParam() {return maxIter;}``` * Use Java anonymous class overriding ```uid()``` to defined the ```uid```, and it solve the second problem described above. * To make the ```getMaxIterParam ``` can be invoked using java reflection, we must make the two class (MyJavaLogisticRegression and MyJavaLogisticRegressionModel) public. so I make them become inner public static class. ## How was this patch tested? Offline tests. Author: Yanbo Liang <ybliang8@gmail.com> Closes #13353 from yanboliang/spark-15605. (cherry picked from commit 4fe7c7b) Signed-off-by: Sean Owen <sowen@cloudera.com>
|
@yanboliang Thanks for tracking down the issue. With your suggested fix, we could leave the existing no-argument constructors for compatibility but deprecate them. Since few people have complained about this so far, I'd be OK with fixing it now or in 2.1. |
What changes were proposed in this pull request?
See SPARK-15605 for the detail of this bug. This PR fix 2 major bugs in this example:
maxIter, it will fail when callingParam.shouldOwn. We need add a public method which return themaxIterObject. BecauseParams.paramsuse java reflection to list all public method whose return type isParam, and invoke them to get all defined param objects in the instance.uidmember defined in Java class will be initialized after Scala traits such asHasFeaturesCol. So whenHasFeaturesColbeing constructed, they getuidwith null, which will causeParam.shouldOwncheck fail.so, here is my changes:
public IntParam getMaxIterParam() {return maxIter;}uid()to defined theuid, and it solve the second problem described above.getMaxIterParamcan be invoked using java reflection, we must make the two class (MyJavaLogisticRegression and MyJavaLogisticRegressionModel) public. so I make them become inner public static class.How was this patch tested?
Offline tests.