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.

Sign up for GitHub

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

Jump to bottom

[SPARK-16107] [R] group glm methods in documentation #13820

Closed
wants to merge 5 commits into from
Closed

[SPARK-16107] [R] group glm methods in documentation #13820

wants to merge 5 commits into from

Conversation

junyangq
Copy link
Contributor

What changes were proposed in this pull request?

This groups GLM methods (spark.glm, summary, print, predict and write.ml) in the documentation. The example code was updated.

How was this patch tested?

N/A

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

screen shot 2016-06-21 at 2 31 37 pm
screen shot 2016-06-21 at 2 31 45 pm

@mengxr
Copy link
Contributor

mengxr commented Jun 21, 2016

ok to test

@mengxr
Copy link
Contributor

mengxr commented Jun 21, 2016

@shivaram @felixcheung I think this significantly improves the current doc. We can add more content to description later. Do you know how to remove the ##D prefixes in the example code?

mengxr
mengxr reviewed Jun 21, 2016
View reviewed changes
R/pkg/R/mllib.R
#'
#' Fits a generalized linear model against a Spark DataFrame.
#' Fit generalized linear model against a Spark DataFrame. Can print, make predictions on the
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Fit -> Fits
  • Can print -> Users can print

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60967 has finished for PR 13820 at commit a99703a.

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

@shivaram
Copy link
Contributor

I think the #D comes from using dontrun https://github.com/wch/r-source/blob/e5b21d0397c607883ff25cca379687b86933d730/src/library/tools/R/Rd2ex.R#L72
I don't see an easy way to disable this yet, but let me dig around a bit more

vectorijk
vectorijk reviewed Jun 21, 2016
View reviewed changes
R/pkg/R/mllib.R
#'
#' Fits a generalized linear model, similarly to R's glm().
#'
#' @title Generalized Linear Models (R-compliant)
Copy link
Contributor

@vectorijk vectorijk Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

As talked with this discussion #13394 (diff), should we follow first sentence as the convention to define the title in this case?

Copy link
Contributor

@vectorijk vectorijk Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Also title changes below? @mengxr

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked @junyangq to try changing #' to # and see whether it works. We still need a comment for the function but we don't want it to appear on the generated Rd doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@junyangq Could you remove @title? We might need an empty line after this line. See attached screenshot:

screen shot 2016-06-21 at 8 26 07 pm

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60974 has finished for PR 13820 at commit 35f0867.

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

@felixcheung
Copy link
Member

I think the ##D are by design - when docs are generated the "preference" is to have self contained tests that would render output and have both the code and output (could be visualization and so on) included in the doc. Since we have dontrun it is annotated differently.

@SparkQA
Copy link

SparkQA commented Jun 22, 2016

Test build #60980 has finished for PR 13820 at commit f5be4a7.

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

mengxr
mengxr reviewed Jun 22, 2016
View reviewed changes
R/pkg/R/mllib.R
#' Fits a generalized linear model, similarly to R's glm().
#'
#' @title Generalized Linear Models (R-compliant)
#' Fits a generalized linear model, similarly to R's glm().
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the extra space between #' and Fits.

felixcheung
felixcheung reviewed Jun 22, 2016
View reviewed changes
R/pkg/R/mllib.R
#' Get the summary of a generalized linear model
#'
#' Returns the summary of a model produced by glm() or spark.glm(), similarly to R's summary().
# Returns the summary of a model produced by glm() or spark.glm(), similarly to R's summary().
Copy link
Member

@felixcheung felixcheung Jun 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I get this is intentional, but I'd suggest adding extra empty newline between # & #' line - it's very easy to newcomers to make mistakes copy/paste lines (and didn't realize it wouldn't output to the doc)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@SparkQA
Copy link

SparkQA commented Jun 22, 2016

Test build #61007 has finished for PR 13820 at commit 8ee701f.

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

@asfgit asfgit closed this in ea3a12b Jun 22, 2016
asfgit pushed a commit that referenced this pull request Jun 22, 2016
@junyangq @mengxr
[SPARK-16107][R] group glm methods in documentation
1cfdd25 
## What changes were proposed in this pull request?

This groups GLM methods (spark.glm, summary, print, predict and write.ml) in the documentation. The example code was updated.

## How was this patch tested?

N/A

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

![screen shot 2016-06-21 at 2 31 37 pm](https://cloud.githubusercontent.com/assets/15318264/16247077/f6eafc04-37bc-11e6-89a8-7898ff3e4078.png)
![screen shot 2016-06-21 at 2 31 45 pm](https://cloud.githubusercontent.com/assets/15318264/16247078/f6eb1c16-37bc-11e6-940a-2b595b10617c.png)

Author: Junyang Qian <[email protected]>
Author: Junyang Qian <[email protected]>

Closes #13820 from junyangq/SPARK-16107.

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

mengxr commented Jun 22, 2016

LGTM. Merged into master and branch-2.0. Thanks!

@junyangq Please follow @felixcheung 's suggestion and insert an empty line between # comments and #' comments in your next PR.

@mengxr mengxr mentioned this pull request Jun 23, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@junyangq @mengxr @SparkQA @shivaram @felixcheung @vectorijk