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.

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-28780][ML] Delete the incorrect setWeightCol method in LinearSVCModel #25510

Closed

Conversation

zhengruifeng
Copy link
Contributor

What changes were proposed in this pull request?

Delete the incorrect method def setWeightCol(value: Double): this.type = set(threshold, value) in LinearSVCModel

Why are the changes needed?

LinearSVCModel should not provide this setter, moreover, this method is wrongly defined.

Does this PR introduce any user-facing change?

yes, a public method is removed

How was this patch tested?

existing suites

@SparkQA
Copy link

SparkQA commented Aug 20, 2019

Test build #109400 has finished for PR 25510 at commit dcaea26.

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

@SparkQA
Copy link

SparkQA commented Aug 20, 2019

Test build #109403 has finished for PR 25510 at commit ce175ca.

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

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Agree, that looks like a copy-paste error. I'm tempted to say we should make it a no-op in 2.x as well, just in case somebody is calling it and accidentally changing the threshold instead. However they'd already be hitting an error if so. We can probably just remove for 3.0.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you, @zhengruifeng and @srowen .

+1, LGTM for 3.0.

Since this API is removed, we need to follow a deprecation process in 2.4.4 and 2.3.4. (cc @kiszk for 2.3.4).

  1. Shall we mark a deprecation annotation on 2.4.4 and 2.3.4?
  2. Make the code no-op and show warning.
  3. Document it in the ML migration guide?

@dongjoon-hyun
Copy link
Member

cc @dbtsai

@srowen
Copy link
Member

srowen commented Aug 20, 2019

That wouldn't hurt to 'back port' a different change that deprecates it and makes it emit a warning only. We can get it into 2.4.4, 2.3.4 indeed. I'm not super concerned about it because a call to setWeightCol("myCol") would not even compile right now. I can't see that anyone would ever have used it. (Yes I already put something in the release notes, but not sure a migration guide entry makes sense.)

@zhengruifeng
Copy link
Contributor Author

zhengruifeng commented Aug 21, 2019

@srowen @dongjoon-hyun I am OK to make another prs for 2.3.4 & 2.4.4.
Maybe two PRs are needed, since the version in deprecation annotation are different.
Thanks for reviewing!

@srowen srowen closed this in 49ffbff Aug 21, 2019
@srowen
Copy link
Member

srowen commented Aug 21, 2019

Merged to master

@dongjoon-hyun
Copy link
Member

Yep. Please make two PR for them, @zhengruifeng . Thanks.

@zhengruifeng zhengruifeng deleted the linearsvc_model_set_weightcol branch August 22, 2019 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants