-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-10258] [DOCUMENTATION, ML] Adding Since() Annotations to ml/feature #8505
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
Conversation
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.
Also annotate the constructor and the public variables in the constructor, e.g., https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/mllib/feature/IDF.scala#L42. Same for other constructors.
|
Looking at the changes, I wonder whether we need to annotate original public methods/variables that have the same since version as the class. Java doesn't do this (http://docs.oracle.com/javase/7/docs/api/java/util/Arrays.html). Please don't remove them now. Just want to discuss and see which approach is more appropriate. +@rxin since this also applies to |
|
We might as well do it. (Any downside?) |
|
The downside would be if we forget since tags in some new methods, it might cause confusions. |
|
I'm for adding them always -- easier to track down which methods don't have them this way. |
|
Okay. @martinbrown Could you add |
|
ping |
|
Ready for another look |
|
ok to test |
|
@martinbrown there are some merge conflicts. could you rebase master? |
|
Test build #42609 has finished for PR 8505 at commit
|
|
Test build #42707 has started for PR 8505 at commit |
|
@mengxr Can you kick off another Jenkins run? It looks like the last run had a fault unrelated to the code. |
|
Test build #42720 has finished for PR 8505 at commit
|
|
Test build #42779 has finished for PR 8505 at commit
|
|
@mengxr any insight on this failure? |
|
Trying again in case it was a transitory download error. Jenkins, retest this please. |
|
@mengxr ping |
|
Test build #43040 has finished for PR 8505 at commit
|
|
test this please |
|
@martinbrown I think initially it was some Jenkins caching problem, then a flaky test. Let's try again. |
|
Test build #43088 has finished for PR 8505 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.
The default constructor should be since 1.2.0. Please check others as well.
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.
Hey @mengxr I'm a little confused here because Binarizer wasn't added until 1.4.0. I did correct the default constructor to 1.2.0 for the original classes from that version (HashingTF, StandardScaler, Tokenizer).
|
LGTM except some minor issues with constructor versions |
|
@martinbrown Do you have time to address my comments? |
|
@mengxr responded |
|
Test build #45184 has finished for PR 8505 at commit
|
|
Test build #45189 has finished for PR 8505 at commit
|
|
@martinbrown It looks like it's been a while since there has been activity. Sorry! Could you please fix the conflicts, and we can see about merging this for 1.6? |
|
Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. |
This PR adds missing `Since` annotations to `ml.feature` package. Closes #8505. ## How was this patch tested? Existing tests. Author: Nick Pentreath <[email protected]> Closes #13641 from MLnick/add-since-annotations.
This PR adds missing `Since` annotations to `ml.feature` package. Closes #8505. ## How was this patch tested? Existing tests. Author: Nick Pentreath <[email protected]> Closes #13641 from MLnick/add-since-annotations. (cherry picked from commit 37494a1) Signed-off-by: Xiangrui Meng <[email protected]>
Unsure what to do for SQLTransformer since it's in master but not in any tagged release.