[SPARK-8920] [MLlib] Add @since tags to mllib.linalg #7729

sabhyankar · 2015-07-28T17:39:24Z

No description provided.

mengxr · 2015-07-28T19:42:17Z

ok to test

mengxr · 2015-07-28T19:42:31Z

@MechCoder Could you help review this PR? Thanks!

SparkQA · 2015-07-28T20:19:36Z

Test build #38755 has finished for PR 7729 at commit 2e5ebd6.

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

MechCoder · 2015-08-01T16:11:45Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

Are you sure that a sealed trait should be tagged?

sabhyankar · 2015-08-01T19:42:18Z

@MechCoder Thanks for pointing that out. I agree that the tag might not be necessary for the sealed trait. Let me know if you come across anything else.

SparkQA · 2015-08-01T20:24:04Z

Test build #39377 has finished for PR 7729 at commit 3be09e9.

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

MechCoder · 2015-08-02T07:23:18Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

It seems that these methods were added to the DenseMatrix object in 1.3.0 right?

MechCoder · 2015-08-02T07:47:29Z

Thanks for the PR. I'm also not sure if tags should be added for hashcode, equals etc since the user is not going to access these features through the documentation.

sabhyankar · 2015-08-04T02:02:16Z

@MechCoder thank you for the very detailed review! I have made most of the changes that you had pointed out except for the following:

I left the tags on the alternative constructor for the public classes as I thought it would be useful to know when it was introduced.
I left the tag in the DistributedMatrix as it is not a sealed trait and would be accessible.

In addition to the above:

I removed the tags from the hashCode and equals methods based on your comment. The only exception is the toString method in the DenseVector and SparseVector classes.

Let me know if you spot something else. Thanks again.

SparkQA · 2015-08-04T02:41:55Z

Test build #39634 has finished for PR 7729 at commit 3012864.

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

MechCoder · 2015-08-04T07:34:25Z

I left the tags on the alternative constructor for the public classes.

Then could you make the tags availble for all the alternative constructors. eg. DenseMatrix

sabhyankar · 2015-08-05T02:51:42Z

@MechCoder I couldnt find the untagged alternate constructor in DenseMatrix. Can you point me to where you see it?

MechCoder · 2015-08-06T07:15:52Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

Hi @MechCoder I sincerely apologize for my ignorance here! I am a little new to Scala and am not sure what I am missing here. I will be happy to fix it if you can provide some clarification. I have fixed the other errors that you identified below and will update the PR.

Oh never mind, I could notice that from the diff view :( . Looks all right to me.

MechCoder · 2015-08-06T07:16:37Z

That's it apart from those couple of comments.

mengxr · 2015-08-11T21:27:41Z

We don't need to add since tags for methods inherited from Java Object like toString, hashCode, and equals. @sabhyankar Could you address @MechCoder 's comments? We should merge this before the 1.5 RC release.

sabhyankar · 2015-08-13T17:48:17Z

I am going to take a look at this and update the PR by tomorrow.

…as tags

mengxr · 2015-08-14T18:33:14Z

test this please

mengxr · 2015-08-14T18:33:26Z

@MechCoder Could you make another pass? Thanks!

sabhyankar · 2015-08-14T18:35:16Z

I removed the tags from two of the toString method and confirmed left the tags in place for the alternate constructors as mechcoder mentioned.

SparkQA · 2015-08-14T19:27:45Z

Test build #40905 has finished for PR 7729 at commit 09aad77.

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

MechCoder · 2015-08-16T05:20:30Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

Should this be 1.2.0?

MechCoder · 2015-08-17T12:58:22Z

LGTM @mengxr

SparkQA · 2015-08-17T15:29:22Z

Test build #41019 has finished for PR 7729 at commit 68b3ed9.

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

Author: Sameer Abhyankar <[email protected]> Author: Sameer Abhyankar <[email protected]> Closes #7729 from sabhyankar/branch_8920. (cherry picked from commit 088b11e) Signed-off-by: Xiangrui Meng <[email protected]>

mengxr · 2015-08-17T23:00:53Z

Merged into master and branch-1.5. Thanks @sabhyankar for adding versions and @MechCoder for code review!

Sameer Abhyankar added 2 commits July 28, 2015 10:29

Add @SInCE tag to mllib.linalg.*

6c4f7a4

Add @SInCE tag to mllib.linalg.*

2e5ebd6

MechCoder reviewed Aug 1, 2015
View reviewed changes

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala Outdated

Copy link

Contributor

MechCoder Aug 1, 2015

Choose a reason for hiding this comment

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

Are you sure that a sealed trait should be tagged?

Remove @SInCE tag from sealed traits Vector and Matrix

3be09e9

MechCoder reviewed Aug 2, 2015
View reviewed changes

Fixed various tagging issues per review by @MechCoder

3012864

MechCoder reviewed Aug 6, 2015
View reviewed changes

Remove @SInCE tag from toString and confirmed alternate constructor h…

09aad77

…as tags

MechCoder reviewed Aug 16, 2015
View reviewed changes

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala Outdated

Copy link

Contributor

MechCoder Aug 16, 2015

Choose a reason for hiding this comment

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

Should this be 1.2.0?

Correct @SInCE tag versions

68b3ed9

asfgit closed this in 088b11e Aug 17, 2015

[SPARK-8920] [MLlib] Add @since tags to mllib.linalg #7729

[SPARK-8920] [MLlib] Add @since tags to mllib.linalg #7729

Uh oh!

Conversation

sabhyankar commented Jul 28, 2015

Uh oh!

mengxr commented Jul 28, 2015

Uh oh!

mengxr commented Jul 28, 2015

Uh oh!

SparkQA commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabhyankar commented Aug 1, 2015

Uh oh!

SparkQA commented Aug 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MechCoder commented Aug 2, 2015

Uh oh!

sabhyankar commented Aug 4, 2015

Uh oh!

SparkQA commented Aug 4, 2015

Uh oh!

MechCoder commented Aug 4, 2015

Uh oh!

sabhyankar commented Aug 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MechCoder commented Aug 6, 2015

Uh oh!

mengxr commented Aug 11, 2015

Uh oh!

sabhyankar commented Aug 13, 2015

Uh oh!

mengxr commented Aug 14, 2015

Uh oh!

mengxr commented Aug 14, 2015

Uh oh!

sabhyankar commented Aug 14, 2015

Uh oh!

SparkQA commented Aug 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MechCoder commented Aug 17, 2015

Uh oh!

SparkQA commented Aug 17, 2015

Uh oh!

mengxr commented Aug 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants