[SPARK-19704][ML] AFTSurvivalRegression should support numeric censorCol #17034
Conversation
|
Test build #73330 has finished for PR 17034 at commit
|
There was a problem hiding this comment.
Technically I guess this could be part of checkNumericTypes similar to checking weight and label cols, but since it is specific to AFT this is ok.
|
As commented we could I guess try to fit in the additional tests into So, this LGTM. |
There was a problem hiding this comment.
maybe you can wrap this in a withClue("Column censor must be of type NumericType but was actually of type StringType") {
...
}
There was a problem hiding this comment.
This place follows the implementation in MLTestingUtils.checkNumericTypes, so I prefer not to change this.
There was a problem hiding this comment.
I think it is discouraged for readability reasons to use _, consider specifying the list of types here
There was a problem hiding this comment.
Yes, I will update this. Thanks for your reviewing!
|
great fix, LGTM! I added minor comments. |
zhengruifeng
force-pushed
the
aft_numeric_censor
branch
from
fd59ca9 to
0185b45
Compare
|
Test build #73731 has finished for PR 17034 at commit
|
| import org.apache.spark.sql.{DataFrame, Row} | ||
| import org.apache.spark.sql.functions.{col, lit} | ||
| import org.apache.spark.sql.types.{ByteType, DecimalType, FloatType, IntegerType, LongType, | ||
| ShortType} |
There was a problem hiding this comment.
The style rule is generally to use _ when you're importing >= 5 things. You can revert it back, thanks!
|
Test build #73738 has started for PR 17034 at commit |
|
Jenkins, retest this please |
|
Test build #73743 has finished for PR 17034 at commit
|
|
Thanks! Merged to master. |
4 participants
What changes were proposed in this pull request?
make
AFTSurvivalRegressionsupport numeric censorColHow was this patch tested?
existing tests and added tests