Skip to content

Comments

[SPARK-28985][PYTHON][ML][FOLLOW-UP] Add _AFTSurvivalRegressionParams#26024

Closed
zero323 wants to merge 1 commit intoapache:masterfrom
zero323:SPARK-28985-FOLLOW-UP-aftsurival-regression
Closed

[SPARK-28985][PYTHON][ML][FOLLOW-UP] Add _AFTSurvivalRegressionParams#26024
zero323 wants to merge 1 commit intoapache:masterfrom
zero323:SPARK-28985-FOLLOW-UP-aftsurival-regression

Conversation

@zero323
Copy link
Member

@zero323 zero323 commented Oct 4, 2019

What changes were proposed in this pull request?

Adds

_AFTSurvivalRegressionParams(HasFeaturesCol, HasLabelCol, HasPredictionCol,
                                   HasMaxIter, HasTol, HasFitIntercept,
                                   HasAggregationDepth): ...

with related Params and uses it to replace HasFitIntercept, HasMaxIter, HasTol and HasAggregationDepth in AFTSurvivalRegression base classes and JavaPredictionModel, in AFTSurvivalRegressionModel base classes.

Why are the changes needed?

Previous work (#25776) on SPARK-28985 replaced JavaEstimator, HasFeaturesCol, HasLabelCol, HasPredictionCol in AFTSurvivalRegression and JavaModel in AFTSurvivalRegressionModel with newly added JavaPredictor:

class JavaPredictor(JavaEstimator, JavaPredictorParams):

and JavaPredictionModel

class JavaPredictionModel(JavaModel, JavaPredictorParams):

respectively.

This however is inconsistent with Scala counterpart where both classes extend private AFTSurvivalRegressionBase

private[regression] trait AFTSurvivalRegressionParams extends Params
with HasFeaturesCol with HasLabelCol with HasPredictionCol with HasMaxIter
with HasTol with HasFitIntercept with HasAggregationDepth with Logging {

This preserves some of the existing inconsistencies (variables as defined in the official example)

from pyspark.ml.regression import AFTSurvivalRegression, AFTSurvivalRegressionModel
from pyspark.ml.param.shared import HasMaxIter, HasTol, HasFitIntercept, HasAggregationDepth
from pyspark.ml.param import Param

issubclass(AFTSurvivalRegressionModel, HasMaxIter)
# False
hasattr(model, "maxIter")  and isinstance(model.maxIter, Param)
# True

issubclass(AFTSurvivalRegressionModel, HasTol)
# False
hasattr(model, "tol")  and isinstance(model.tol, Param)
# True

and can cause problems in the future, if Predictor / PredictionModel API changes (unlike IsotonicRegression, current implementation is technically speaking correct, though incomplete).

Does this PR introduce any user-facing change?

Yes, it adds a number of base classes to AFTSurvivalRegressionModel. These change purely additive and have negligible potential for breaking existing code (and none, compared to changes already made in #25776). Additionally affected API hasn't been released in the current form yet.

How was this patch tested?

  • Existing unit tests.
  • Manual testing.

CC @huaxingao, @zhengruifeng

@SparkQA
Copy link

SparkQA commented Oct 4, 2019

Test build #111776 has finished for PR 26024 at commit 3279f9f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class _AFTSurvivalRegressionParams(HasFeaturesCol, HasLabelCol, HasPredictionCol,
  • class AFTSurvivalRegression(JavaEstimator, _AFTSurvivalRegressionParams,
  • class AFTSurvivalRegressionModel(JavaModel, _AFTSurvivalRegressionParams,

@zhengruifeng
Copy link
Contributor

Great Catch. LGTM

@huaxingao
Copy link
Contributor

LGTM

@srowen
Copy link
Member

srowen commented Oct 4, 2019

Merged to master

@srowen srowen closed this in df22535 Oct 4, 2019
@zero323
Copy link
Member Author

zero323 commented Oct 4, 2019

Thanks @srowen, @huaxingao, @zhengruifeng!

@zero323 zero323 deleted the SPARK-28985-FOLLOW-UP-aftsurival-regression branch October 4, 2019 23:15
zero323 added a commit to zero323/pyspark-stubs that referenced this pull request Oct 5, 2019
* Reflect changes introduced with SPARK-28985

* Add _IsotonicRegressionBase - apache/spark#26023

* Add _AFTSurvivalRegressionParams - apache/spark#26024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants