Skip to content

[SPARK-16143][R] group AFT survival regression methods docs in a single Rd #13927

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
Closed
junyangq wants to merge 4 commits into from
Closed

[SPARK-16143][R] group AFT survival regression methods docs in a single Rd #13927

junyangq wants to merge 4 commits into from

Conversation

@junyangq
Copy link
Contributor

@junyangq junyangq commented Jun 27, 2016

What changes were proposed in this pull request?

This PR groups spark.survreg, summary(AFT), predict(AFT), write.ml(AFT) for survival regression into a single Rd.

How was this patch tested?

Manually checked generated HTML doc. See attached screenshots.

screen shot 2016-06-27 at 10 28 20 am
screen shot 2016-06-27 at 10 28 35 am

junyangq
junyangq reviewed Jun 27, 2016
View reviewed changes
R/pkg/R/mllib.R Outdated
Copy link
Contributor Author

@junyangq junyangq Jun 27, 2016

Choose a reason for hiding this comment

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

flip the order - otherwise the newData parameter doesn't show in the generated doc.

@mengxr
Copy link
Contributor

mengxr commented Jun 27, 2016

add to whitelist

@mengxr
Copy link
Contributor

mengxr commented Jun 27, 2016

ok to test

mengxr
mengxr reviewed Jun 27, 2016
View reviewed changes
R/pkg/R/mllib.R Outdated
Copy link
Contributor

@mengxr mengxr Jun 27, 2016

Choose a reason for hiding this comment

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

no "." at the end

@SparkQA
Copy link

SparkQA commented Jun 27, 2016

Test build #61315 has finished for PR 13927 at commit 2b7f8e2.

  • This patch fails R style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

mengxr
mengxr reviewed Jun 27, 2016
View reviewed changes
R/pkg/R/mllib.R Outdated
Copy link
Contributor

@mengxr mengxr Jun 27, 2016

Choose a reason for hiding this comment

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

Please do not move code blocks unless it is necessary. It makes it hard to review the changes and causes conflicts with other PRs. If we want to re-organize the ordering, we should do that in a separate PR without any real code changes.

@mengxr
Copy link
Contributor

mengxr commented Jun 27, 2016

@junyangq You can use dev/lint-r to check R code style on your local machine.

@junyangq
Copy link
Contributor Author

junyangq commented Jun 27, 2016

Updated screenshots.

screen shot 2016-06-27 at 1 53 57 pm
screen shot 2016-06-27 at 1 54 05 pm

@SparkQA
Copy link

SparkQA commented Jun 27, 2016

Test build #61324 has finished for PR 13927 at commit 47e9ab9.

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

@SparkQA
Copy link

SparkQA commented Jun 27, 2016

Test build #61326 has finished for PR 13927 at commit 361bce8.

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

mengxr
mengxr reviewed Jun 27, 2016
View reviewed changes
R/pkg/R/mllib.R Outdated
# spark.survreg, similarly to R package survival's predict.

#' @param newData A SparkDataFrame for testing
#' @return \code{predict} returns a SparkDataFrame containing predicted values (median of
Copy link
Contributor

@mengxr mengxr Jun 27, 2016

Choose a reason for hiding this comment

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

Had an offline discussion with @junyangq and confirmed that this is response at the original scale (mean predicted value at scale = 1.0) rather than median of the survival time.

@mengxr
Copy link
Contributor

mengxr commented Jun 27, 2016

LGTM except one minor issue with predict.

@SparkQA
Copy link

SparkQA commented Jun 28, 2016

Test build #61334 has finished for PR 13927 at commit 7dc2912.

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

@mengxr
Copy link
Contributor

mengxr commented Jun 28, 2016

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

asfgit pushed a commit that referenced this pull request Jun 28, 2016
@junyangq @mengxr
[SPARK-16143][R] group AFT survival regression methods docs in a sing…
4c4f777 
…le Rd

## What changes were proposed in this pull request?

This PR groups `spark.survreg`, `summary(AFT)`, `predict(AFT)`, `write.ml(AFT)` for survival regression into a single Rd.

## How was this patch tested?

Manually checked generated HTML doc. See attached screenshots.

![screen shot 2016-06-27 at 10 28 20 am](https://cloud.githubusercontent.com/assets/15318264/16392008/a14cf472-3c5e-11e6-9ce5-490ed1a52249.png)
![screen shot 2016-06-27 at 10 28 35 am](https://cloud.githubusercontent.com/assets/15318264/16392009/a14e333c-3c5e-11e6-8bd7-c2e9ba71f8e2.png)

Author: Junyang Qian <[email protected]>

Closes #13927 from junyangq/SPARK-16143.

(cherry picked from commit 1b7fc58)
Signed-off-by: Xiangrui Meng <[email protected]>
@asfgit asfgit closed this in 1b7fc58 Jun 28, 2016
@mengxr mengxr mentioned this pull request Jun 28, 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.

3 participants

@junyangq @mengxr @SparkQA