[SPARK-30377][ML] Make Regressors extend abstract class Regressor #27168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
huaxingao
commented
Jan 10, 2020
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegressionModel.setPredictionCol"), | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setFeaturesCol"), | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setLabelCol"), | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setPredictionCol") | ||
| ) |
There was a problem hiding this comment.
Changing hierarchy of AFTSurvivalRegression/AFTSurvivalRegressionModelfrom extending Estimator/Model to Regressor/RegressionModel caused the following MiMa errors:
[error] * method setFeaturesCol(java.lang.String)org.apache.spark.ml.regression.AFTSurvivalRegressionModel in class org.apache.spark.ml.regression.AFTSurvivalRegressionModel has a different result type in current version, where it is org.apache.spark.ml.PredictionModel rather than org.apache.spark.ml.regression.AFTSurvivalRegressionModel
[error] filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegressionModel.setFeaturesCol")
[error] * method setPredictionCol(java.lang.String)org.apache.spark.ml.regression.AFTSurvivalRegressionModel in class org.apache.spark.ml.regression.AFTSurvivalRegressionModel has a different result type in current version, where it is org.apache.spark.ml.PredictionModel rather than org.apache.spark.ml.regression.AFTSurvivalRegressionModel
[error] filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegressionModel.setPredictionCol")
[error] * method fit(org.apache.spark.sql.Dataset)org.apache.spark.ml.regression.AFTSurvivalRegressionModel in class org.apache.spark.ml.regression.AFTSurvivalRegression has a different result type in current version, where it is org.apache.spark.ml.Model rather than org.apache.spark.ml.regression.AFTSurvivalRegressionModel
[error] filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.fit")
[error] * method setFeaturesCol(java.lang.String)org.apache.spark.ml.regression.AFTSurvivalRegression in class org.apache.spark.ml.regression.AFTSurvivalRegression has a different result type in current version, where it is org.apache.spark.ml.Predictor rather than org.apache.spark.ml.regression.AFTSurvivalRegression
[error] filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setFeaturesCol")
[error] * method setLabelCol(java.lang.String)org.apache.spark.ml.regression.AFTSurvivalRegression in class org.apache.spark.ml.regression.AFTSurvivalRegression has a different result type in current version, where it is org.apache.spark.ml.Predictor rather than org.apache.spark.ml.regression.AFTSurvivalRegression
[error] filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setLabelCol")
[error] * method setPredictionCol(java.lang.String)org.apache.spark.ml.regression.AFTSurvivalRegression in class org.apache.spark.ml.regression.AFTSurvivalRegression has a different result type in current version, where it is org.apache.spark.ml.Predictor rather than org.apache.spark.ml.regression.AFTSurvivalRegression
[error] filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setPredictionCol")
There is not any API change, though.
Changing the hierarchy of extending Predictor/PredictionModel to Regressor/RegressionModel doesn't cause MiMa problem.
There was a problem hiding this comment.
Yeah same story as last time eh, the type should be identical but it's not super obvious to Mima that this.type is the same as type T = [the model type].
srowen
approved these changes
Jan 10, 2020
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegressionModel.setPredictionCol"), | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setFeaturesCol"), | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setLabelCol"), | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.regression.AFTSurvivalRegression.setPredictionCol") | ||
| ) |
There was a problem hiding this comment.
Yeah same story as last time eh, the type should be identical but it's not super obvious to Mima that this.type is the same as type T = [the model type].
|
Jenkins test this please |
|
Test build #116499 has finished for PR 27168 at commit
|
|
Test build #116502 has finished for PR 27168 at commit
|
viirya
reviewed
Jan 11, 2020
| @@ -208,8 +195,7 @@ class AFTSurvivalRegression @Since("1.6.0") (@Since("1.6.0") override val uid: S | |||
| } | |||
|
|
|||
| @Since("2.0.0") | |||
There was a problem hiding this comment.
We change from fit to train. Strictly it is not since 2.0.0.
There was a problem hiding this comment.
Changed to 3.0.0. Thanks!
viirya
approved these changes
Jan 11, 2020
|
Test build #116536 has finished for PR 27168 at commit
|
|
Merged to master |
Closed
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Make Regressors extend abstract class Regressor:
AFTSurvivalRegression extends Estimator => extends RegressorDecisionTreeRegressor extends Predictor => extends RegressorFMRegressor extends Predictor => extends RegressorGBTRegressor extends Predictor => extends RegressorRandomForestRegressor extends Predictor => extends RegressorWe will not make
IsotonicRegressionextendRegressorbecause it is tricky to handle both DoubleType and VectorType.Why are the changes needed?
Make class hierarchy consistent for all Regressors
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing tests