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-12903] [SparkR] Add covar_samp and covar_pop for SparkR #10829

Closed
wants to merge 5 commits into from

Conversation

yanboliang
Copy link
Contributor

Add covar_samp and covar_pop for SparkR.
Should we also provide cov alias for covar_samp? There is cov implementation at stats.R which masks stats::cov already, but may bring to breaking API change.

cc @sun-rui @felixcheung @shivaram

@yanboliang
Copy link
Contributor Author

Jenkins, test this please.

@SparkQA
Copy link

SparkQA commented Jan 19, 2016

Test build #49680 has finished for PR 10829 at commit 7c3e718.

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

@@ -424,6 +424,14 @@ setGeneric("cov", function(x, col1, col2) {standardGeneric("cov") })
#' @export
setGeneric("corr", function(x, ...) {standardGeneric("corr") })

#' @rdname statfunctions
#' @export
setGeneric("covar_samp", function(x, ...) {standardGeneric("covar_samp") })
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of parameters are fixed to be 2. No need to support varargs.
Something like:
setGeneric("covar_samp", function(col1, col2) {standardGeneric("covar_samp") })

@sun-rui
Copy link
Contributor

sun-rui commented Jan 19, 2016

@yanboliang, please add a new method for "cov", which is alias to "covar_samp"

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49898 has finished for PR 10829 at commit ebde36a.

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

@yanboliang
Copy link
Contributor Author

@sun-rui Thanks for your comments. There is a cov implementation at stats.R with different signature:

setMethod("cov",
          signature(x = "DataFrame", col1 = "character", col2 = "character"),
          function(x, col1, col2) {
            statFunctions <- callJMethod(x@sdf, "stat")
            callJMethod(statFunctions, "cov", col1, col2)
          })

And this cov has masked stats::cov, so we can not add cov as alias to covar_samp.

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49900 has finished for PR 10829 at commit 16d3fad.

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

@sun-rui
Copy link
Contributor

sun-rui commented Jan 22, 2016

@yanboliang, we can have different methods for a generic function. You can try something like follows:

setGeneric("cov", function(x, ...) {standardGeneric("cov") })

setMethod("cov",
          signature(x = "DataFrame", ...),

setMethod("cov",
          signature(x = "characterOrColumn", ...),

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49913 has finished for PR 10829 at commit eca64a6.

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

@SparkQA
Copy link

SparkQA commented Jan 24, 2016

Test build #49954 has finished for PR 10829 at commit a036f95.

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

@sun-rui
Copy link
Contributor

sun-rui commented Jan 25, 2016

LGTM

@felixcheung
Copy link
Member

looks good

@shivaram
Copy link
Contributor

LGTM. Thanks @yanboliang - Merging this to master

@asfgit asfgit closed this in e7f9199 Jan 27, 2016
@yanboliang yanboliang deleted the spark-12903 branch January 27, 2016 03:39
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.

5 participants