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-9175] [MLlib] BLAS.gemm fails to update matrix C when alpha==0 and beta!=1 #7503

Closed
wants to merge 1 commit into from
Closed

Conversation

rotationsymmetry
Copy link
Contributor

Fix BLAS.gemm to update matrix C when alpha==0 and beta!=1
Also include unit tests to verify the fix.

@mengxr @brkyvz

@srowen
Copy link
Member

srowen commented Jul 19, 2015

LGTM

@srowen
Copy link
Member

srowen commented Jul 19, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Jul 19, 2015

Test build #37768 has finished for PR 7503 at commit fce199c.

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

@srowen
Copy link
Member

srowen commented Jul 20, 2015

@brkyvz is this OK with you?

@brkyvz
Copy link
Contributor

brkyvz commented Jul 20, 2015

Can we add the same check to gemv please? In addition, instead of calling the dgemm routine to just scale C, maybe we can just call f2jBlas.scal? @mengxr What do you think?

@rotationsymmetry
Copy link
Contributor Author

@brkyvz

  1. I will put in the same check to gemv.
  2. scal for (dense or sparse) matrices will be helpful. I could implement it as well. Shall I open a new JIRA/PR?

cc @srowen @mengxr

@mengxr
Copy link
Contributor

mengxr commented Jul 20, 2015

ok to test

@mengxr
Copy link
Contributor

mengxr commented Jul 21, 2015

I like @brkyvz 's idea about calling scal when alpha == 0. It would be a noop when beta == 0, but handled by scal. Since this is a bug, I would like to merge this first and have follow-up JIRAs to implement scal.

asfgit pushed a commit that referenced this pull request Jul 21, 2015
… and beta!=1

Fix BLAS.gemm to update matrix C when alpha==0 and beta!=1
Also include unit tests to verify the fix.

mengxr brkyvz

Author: Meihua Wu <[email protected]>

Closes #7503 from rotationsymmetry/fix_BLAS_gemm and squashes the following commits:

fce199c [Meihua Wu] Fix BLAS.gemm to update C when alpha==0 and beta!=1

(cherry picked from commit ff3c72d)
Signed-off-by: Xiangrui Meng <[email protected]>
asfgit pushed a commit that referenced this pull request Jul 21, 2015
… and beta!=1

Fix BLAS.gemm to update matrix C when alpha==0 and beta!=1
Also include unit tests to verify the fix.

mengxr brkyvz

Author: Meihua Wu <[email protected]>

Closes #7503 from rotationsymmetry/fix_BLAS_gemm and squashes the following commits:

fce199c [Meihua Wu] Fix BLAS.gemm to update C when alpha==0 and beta!=1

(cherry picked from commit ff3c72d)
Signed-off-by: Xiangrui Meng <[email protected]>
@asfgit asfgit closed this in ff3c72d Jul 21, 2015
asfgit pushed a commit that referenced this pull request Jul 21, 2015
… and beta!=1

Fix BLAS.gemm to update matrix C when alpha==0 and beta!=1
Also include unit tests to verify the fix.

mengxr brkyvz

Author: Meihua Wu <[email protected]>

Closes #7503 from rotationsymmetry/fix_BLAS_gemm and squashes the following commits:

fce199c [Meihua Wu] Fix BLAS.gemm to update C when alpha==0 and beta!=1

(cherry picked from commit ff3c72d)
Signed-off-by: Xiangrui Meng <[email protected]>
@mengxr
Copy link
Contributor

mengxr commented Jul 21, 2015

Merged into master and all branches since 1.2. Thanks!

@SparkQA
Copy link

SparkQA commented Jul 21, 2015

Test build #37872 has finished for PR 7503 at commit fce199c.

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

@rotationsymmetry
Copy link
Contributor Author

Hi @mengxr, thank you for merge. I have also fixed a similar bug in BLAS.gemv, as suggested by @brkyvz . Shall I make another pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants