Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-32310][ML][PySpark] ML params default value parity in feature and tuning #29153

Closed
wants to merge 4 commits into from
Closed

[SPARK-32310][ML][PySpark] ML params default value parity in feature and tuning #29153

wants to merge 4 commits into from

Conversation

huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

set params default values in trait Params for feature and tuning in both Scala and Python.

Why are the changes needed?

Make ML has the same default param values between estimator and its corresponding transformer, and also between Scala and Python.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing and modified tests

@SparkQA
Copy link

SparkQA commented Jul 19, 2020

Test build #126117 has finished for PR 29153 at commit 86e0579.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@huaxingao huaxingao mentioned this pull request Jul 20, 2020
Closed
@SparkQA
Copy link

SparkQA commented Jul 21, 2020

Test build #126220 has finished for PR 29153 at commit 1586e30.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 21, 2020

Test build #126265 has finished for PR 29153 at commit 5e09d2d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@huaxingao huaxingao changed the title [SPARK-32310][ML][PySpark][WIP] ML params default value parity in feature and tuning [SPARK-32310][ML][PySpark] ML params default value parity in feature and tuning Jul 21, 2020
@SparkQA
Copy link

SparkQA commented Jul 22, 2020

Test build #126307 has finished for PR 29153 at commit 46451b6.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@huaxingao
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Jul 22, 2020

Test build #126343 has finished for PR 29153 at commit 46451b6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@huaxingao
Copy link
Contributor Author

cc @srowen @viirya @zhengruifeng

@srowen
Copy link
Member

srowen commented Jul 24, 2020

Looks good if there are no API or behavior changes (that are not bug fixes).

zhengruifeng
zhengruifeng approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@zhengruifeng zhengruifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed twice, it LGTM if related places were all changed.

@huaxingao
Copy link
Contributor Author

retest this please

viirya
viirya reviewed Jul 28, 2020
View reviewed changes
python/pyspark/ml/regression.py
@@ -2391,7 +2404,7 @@ def __repr__(self):


class _FactorizationMachinesParams(_PredictorParams, HasMaxIter, HasStepSize, HasTol,
HasSolver, HasSeed, HasFitIntercept, HasRegParam):
HasSolver, HasSeed, HasFitIntercept, HasRegParam, HasWeightCol):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add HasWeightCol? Seems you don't add a default of weight col?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viirya Thanks for reviewing. I added HasWeightCol in FactorizationMachinesParams in Scala in #28960 when I added FMClassificationSummary. ClassificationSummary needs weightCol, https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/ml/classification/ClassificationSummary.scala#L48. FMClassifier/Regressor doesn't support instance weight yet, so I didn't add setWeightCol or set default value in FactorizationMachinesParams.

viirya
viirya reviewed Jul 28, 2020
View reviewed changes
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for one question.

@SparkQA
Copy link

SparkQA commented Jul 28, 2020

Test build #126724 has finished for PR 29153 at commit 46451b6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Aug 3, 2020

I think you can merge when ready

@huaxingao huaxingao closed this in bc78859 Aug 3, 2020
@huaxingao
Copy link
Contributor Author

Merged to master. Thank you all for reviewing!

@huaxingao huaxingao deleted the default2 branch August 3, 2020 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viirya viirya viirya left review comments

@srowen srowen srowen approved these changes

@zhengruifeng zhengruifeng zhengruifeng approved these changes

Assignees
No one assigned
Labels
ML PYTHON
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@huaxingao @SparkQA @srowen @viirya @zhengruifeng