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-19389][ML][PYTHON][DOC] Minor doc fixes for ML Python Params and LinearSVC #16723

Closed
wants to merge 2 commits into from

Conversation

jkbradley
Copy link
Member

What changes were proposed in this pull request?

  • Removed Since tags in Python Params since they are inherited by other classes
  • Fixed doc links for LinearSVC

How was this patch tested?

  • doc tests
  • generating docs locally and checking manually

@jkbradley
Copy link
Member Author

@wangmiao1981 Would you mind checking this? It has small fixes I noticed when reviewing your PR for Python LinearSVC.

@SparkQA
Copy link

SparkQA commented Jan 28, 2017

Test build #72094 has finished for PR 16723 at commit fa522e6.

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

@@ -47,7 +47,7 @@ private[classification] trait LinearSVCParams extends ClassifierParams with HasR
/**
* :: Experimental ::
*
* Linear SVM Classifier (https://en.wikipedia.org/wiki/Support_vector_machine#Linear_SVM)
* [[https://en.wikipedia.org/wiki/Support_vector_machine#Linear_SVM Linear SVM Classifier]]
Copy link
Member

@HyukjinKwon HyukjinKwon Jan 28, 2017

Choose a reason for hiding this comment

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

Would you mind changing it to

 * <a href="https://en.wikipedia.org/wiki/Support_vector_machine#Linear_SVM">
 * Linear SVM Classifier</a>

? It seems scala link annotation breaks Javadoc8 as below:

[error] .../spark/mllib/target/java/org/apache/spark/ml/classification/LinearSVC.java:5: error: unexpected text
[error]  * {@link https://en.wikipedia.org/wiki/Support_vector_machine#Linear_SVM  Linear SVM Classifier}

I tested this via ./build/sbt unidoc with Java 8.

(Currently, it seems there are already few errors. Let me fix them with other warnings in near future).
+Just FYI, there are few cases I have found and fixed in #16013

Copy link
Contributor

@wangmiao1981 wangmiao1981 Jan 30, 2017

Choose a reason for hiding this comment

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

@jkbradley I checked several other scala files like, GBTRegressor.scala, DecisonTreeRegressor.scala etc. URLs are in the format of <a href>...</a>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! will do

@wangmiao1981
Copy link
Contributor

@jkbradley Sorry for replying late! I was out for Chinese new year this weekend. I will check the issue mentioned by HyukjinKwon otherwise, it looks good to me.

@jkbradley
Copy link
Member Author

I delayed too! I just pushed a fix. I couldn't test it since it looks like the Java 8 doc gen has already been broken again. (Thanks a lot for the efforts to fix it! Btw, are you pinging the people who break it whenever you see issues?)

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Feb 2, 2017

Ah, I just fixed the others together in #16741 It is broken again.. Yes, I understand it is super easy to break. So I am trying to inform committers or frequent contributors purely as a kind comment so that we could avoid and fix them by ourself at our best (not at all blaming purpose).

(IMHO, at least for now, building javadoc everytime might be good to do but not required. We can avoid them at our best in our PRs and then sweep them when the release is close or in other related PRs if there are)

@SparkQA
Copy link

SparkQA commented Feb 2, 2017

Test build #72265 has finished for PR 16723 at commit ffd4756.

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

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Feb 2, 2017

(I just rebased it based on this PR and built the javadoc8 for sure. I believe it should show an error in those newly introduced errors if this PR introduce the break but it seems not. So, LGTM for doc changes.)

@jkbradley
Copy link
Member Author

OK thanks a lot @HyukjinKwon and @wangmiao1981 !
Merging with master

@asfgit asfgit closed this in 1d5d2a9 Feb 2, 2017
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
…nd LinearSVC

## What changes were proposed in this pull request?

* Removed Since tags in Python Params since they are inherited by other classes
* Fixed doc links for LinearSVC

## How was this patch tested?

* doc tests
* generating docs locally and checking manually

Author: Joseph K. Bradley <[email protected]>

Closes apache#16723 from jkbradley/pyparam-fix-doc.
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