Skip to content

Conversation

@hhbyyh
Copy link
Contributor

@hhbyyh hhbyyh commented Sep 9, 2015

jira: https://issues.apache.org/jira/browse/SPARK-10491

We implemented dspr with sparse vector support in RowMatrix. This method is also used in WeightedLeastSquares and other places. It would be useful to move it to linalg.BLAS.

Let me know if new UT needed.

@holdenk
Copy link
Contributor

holdenk commented Sep 9, 2015

Since the JIRA references the reason for moving this being sharing code with some other components, maybe it would be good to have those other components also the method?

@SparkQA
Copy link

SparkQA commented Sep 9, 2015

Test build #42181 has finished for PR 8663 at commit f27dbf3.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Should be spr instead of dspr. We only use double precision.
  • It might be useful to make U a DenseVector (or add a convenient method) for API consistency.
  • Do you mind adding tests for both dense and sparse input?

@mengxr
Copy link
Contributor

mengxr commented Sep 9, 2015

@hhbyyh I'm going to merge #8588 first. Could you please update this PR after? Thanks!

@mengxr
Copy link
Contributor

mengxr commented Sep 14, 2015

@hhbyyh #8588 was merged. Do you have time to update this PR? Thanks!

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Sep 15, 2015

@mengxr Sorry for the delay. working on it now.

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Sep 15, 2015

@mengxr Thanks for the review. Updated.

@mengxr
Copy link
Contributor

mengxr commented Sep 15, 2015

LGTM pending Jenkins

@SparkQA
Copy link

SparkQA commented Sep 15, 2015

Test build #42491 has finished for PR 8663 at commit f8f2633.

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

@mengxr
Copy link
Contributor

mengxr commented Sep 15, 2015

Merged into master. Thanks!

@asfgit asfgit closed this in c35fdcb Sep 15, 2015
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.

4 participants