-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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.
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
[SPARK-29093][PYTHON][ML] Remove automatically generated param setters in _shared_params_code_gen.py #26232
Conversation
…s in _shared_params_code_gen.py
Test build #112559 has finished for PR 26232 at commit
|
Test build #112571 has finished for PR 26232 at commit
|
Test build #112583 has finished for PR 26232 at commit
|
1, why you remove the |
@zhengruifeng For the newly added setters in Transformers, most of them are new in 3.0.0. I will double check to make sure all the newly added one have I guess I will only add |
Test build #112610 has finished for PR 26232 at commit
|
merged to master, thanks @huaxingao ! |
Thanks a lot! @zhengruifeng |
""" | ||
Sets the value of :py:attr:`topicDistributionCol`. | ||
|
||
>>> algo = LDA().setTopicDistributionCol("topicDistributionCol") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anything, shouldn't we have model specific dosctring here? This one just duplicates what we already have in estimator:
spark/python/pyspark/ml/clustering.py
Lines 1159 to 1160 in b389b8c
>>> algo = LDA().setTopicDistributionCol("topicDistributionCol") | |
>>> algo.getTopicDistributionCol() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in the middle of rechecking this PR and other ml parity PRs. I will have a follow up PR soon to fix this along with a few other problems I found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @huaxingao. If you don't mind me asking ‒ should I expect any serious shifts?
(I am in the middle of handling annotations for this, and it might be useful to know what to expect)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Just fix the small things I overlooked before, for example, I have setters in TrainValidationSplitModel/CrossValidatorModel
because these setters are used in _from_java
. I will remove the setters and use _set instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @huaxingao.
return self._set(outputCol=value) | ||
|
||
@since("3.0.0") | ||
def setMinTF(self, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is already present in 2.4 (L916-L921)
return self._set(minTF=value) | ||
|
||
@since("3.0.0") | ||
def setBinary(self, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, L923-L928
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix were in a few hours ago. Please let me know if you see other problems. Thanks! @zero323
What changes were proposed in this pull request?
Remove automatically generated param setters in _shared_params_code_gen.py
Why are the changes needed?
To keep parity between scala and python
Does this PR introduce any user-facing change?
Yes
Add some setters in Python ML XXXModels
How was this patch tested?
unit tests