-
Notifications
You must be signed in to change notification settings - Fork 29.1k
[SPARK-35678][ML][FOLLOWUP] softmax support offset and step #32991
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-35678][ML][FOLLOWUP] softmax support offset and step #32991
Conversation
|
Test build #140058 has finished for PR 32991 at commit
|
|
Kubernetes integration test starting |
fb44753 to
73d11ff
Compare
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Test build #140064 has finished for PR 32991 at commit
|
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.
@srowen @huaxingao This change makes MultilayerPerceptronClassifierTest in test_algorithms.py fail.
To pass the test, I need to use while loop instead of dscal here.
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.
Oh OK, is it just that the answer is slightly different with dscal? would it be reasonable to loosen a tolerance? but this is fine.
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.
existing result is:
master:
Row(features=DenseVector([0.1, 0.1, 0.25, 0.25]), rawPrediction=DenseVector([-11.6082, -8.1583, 22.1776]), probability=DenseVector([0.0, 0.0, 1.0]), prediction=2.0)
if we use dscal here, then
Row(features=DenseVector([0.1, 0.1, 0.25, 0.25]), rawPrediction=DenseVector([-11.824, -8.298, 22.5299]), probability=DenseVector([0.0, 0.0, 1.0]), prediction=2.0)
maybe OK to change the pytest
|
Kubernetes integration test status success |
|
link #32927 |
73d11ff to
a254a83
Compare
|
Kubernetes integration test starting |
|
Test build #140110 has finished for PR 32991 at commit
|
|
Kubernetes integration test status failure |
|
Thanks, merged to master! |
|
Thanks all! |
|
quick question: can this be related to this flaky test? I just saw it once at https://github.com/apache/spark/runs/2900969845 but might be worth noting here. |
|
@HyukjinKwon it seems related to this pr. @srowen @huaxingao should we remove this pytest or revert the usage of |
|
I would revert the ANN change - something's too dependent on the math and ordering of operations or something. If we can just revert that and see it work in a follow up PR, that's a way forward; this may not even fail every time in master as I think it's simply flaky ATM |
|
ok, I will send a pr to revert the change in ANN |
|
Pardon if I missed it but was this partly reverted? |
|
Oh, yeah it was reverted partially already at #33049 |
What changes were proposed in this pull request?
softmax support offset and step, then we can use it in ANN and NB
Why are the changes needed?
to simplify impl
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing testsuite