Skip to content

[SPARK-29269][PYTHON][ML] Pyspark ALSModel support getters/setters#25947

Closed
huaxingao wants to merge 4 commits intoapache:masterfrom
huaxingao:spark-29269
Closed

[SPARK-29269][PYTHON][ML] Pyspark ALSModel support getters/setters#25947
huaxingao wants to merge 4 commits intoapache:masterfrom
huaxingao:spark-29269

Conversation

@huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

Add getters/setters in Pyspark ALSModel.

Why are the changes needed?

To keep parity between python and scala.

Does this PR introduce any user-facing change?

Yes.
add the following getters/setters to ALSModel

get/setUserCol
get/setItemCol
get/setColdStartStrategy
get/setPredictionCol

How was this patch tested?

add doctest

@SparkQA
Copy link

SparkQA commented Sep 27, 2019

Test build #111468 has finished for PR 25947 at commit e2850cc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class _ALSModelParams(HasPredictionCol):
  • class _ALSParams(_ALSModelParams, HasMaxIter, HasRegParam, HasCheckpointInterval, HasSeed):
  • class ALS(JavaEstimator, _ALSParams, JavaMLWritable, JavaMLReadable):
  • class ALSModel(JavaModel, _ALSModelParams, JavaMLWritable, JavaMLReadable):

*/
private[recommendation] trait ALSParams extends ALSModelParams with HasMaxIter with HasRegParam
with HasPredictionCol with HasCheckpointInterval with HasSeed {
with HasCheckpointInterval with HasSeed {
Copy link
Member

Choose a reason for hiding this comment

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

Because ALSModelParams already has HasPredictionCol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It seems to be redundant to me.

"""

@since("1.4.0")
def setUserCol(self, value):
Copy link
Member

Choose a reason for hiding this comment

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

These are newly added, right? So which one is more correct, since(1.4.0) or since(3.0.0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be since(3.0.0). Thanks for catching that problem.

@SparkQA
Copy link

SparkQA commented Sep 28, 2019

Test build #111505 has finished for PR 25947 at commit 875f9fb.

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

@SparkQA
Copy link

SparkQA commented Oct 3, 2019

Test build #111748 has finished for PR 25947 at commit be2037e.

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

@SparkQA
Copy link

SparkQA commented Oct 3, 2019

Test build #111758 has finished for PR 25947 at commit 1d1f041.

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

@zhengruifeng
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Oct 8, 2019

Test build #111866 has finished for PR 25947 at commit 1d1f041.

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

@zhengruifeng
Copy link
Contributor

Merged to master. Thanks all!

@huaxingao
Copy link
Contributor Author

Thanks! @viirya @zhengruifeng

@huaxingao huaxingao deleted the spark-29269 branch October 8, 2019 16:30
atronchi pushed a commit to atronchi/spark that referenced this pull request Oct 23, 2019
### What changes were proposed in this pull request?
Add getters/setters in Pyspark ALSModel.

### Why are the changes needed?
To keep parity between python and scala.

### Does this PR introduce any user-facing change?
Yes.
add the following getters/setters to ALSModel
```
get/setUserCol
get/setItemCol
get/setColdStartStrategy
get/setPredictionCol
```

### How was this patch tested?
add doctest

Closes apache#25947 from huaxingao/spark-29269.

Authored-by: Huaxin Gao <huaxing@us.ibm.com>
Signed-off-by: zhengruifeng <ruifengz@foxmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants