-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-19827][R][FOLLOWUP] spark.ml R API for PIC #23292
Conversation
@srowen Sorry for the problems and thanks for the PR. I am actually working on the follow up PR, but I wasn't fast enough. |
R/pkg/R/mllib_clustering.R
Outdated
@@ -621,38 +621,35 @@ setMethod("write.ml", signature(object = "LDAModel", path = "character"), | |||
#' | |||
#' A scalable graph clustering algorithm. Users can call \code{spark.assignClusters} to | |||
#' return a cluster assignment for each input vertex. | |||
#' | |||
# Run the PIC algorithm and returns a cluster assignment for each input vertex. |
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.
sorry I missed the '. should be #'
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.
I mean line 624
#' \code{sequence: ArrayType(ArrayType(T))} (T is the item type) | ||
#' \code{freq: Long} | ||
#' \code{sequence: ArrayType(ArrayType(T))}, \code{freq: integer} | ||
#' where T is the item type |
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.
I know there is a problem here too. But the one that needs to get fixed is in line 637 in R/pkg/R/mllib_clustering.R.
I guess I will have a separate follow up PR to fix the mllib_fpm.R problem. Sorry for the mess.
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.
Yeah I wanted to fix both here while we're at it.
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.
let's fix both
Test build #99988 has finished for PR 23292 at commit
|
Test build #99990 has finished for PR 23292 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.
LGTM thx @srowen
#' \code{sequence: ArrayType(ArrayType(T))} (T is the item type) | ||
#' \code{freq: Long} | ||
#' \code{sequence: ArrayType(ArrayType(T))}, \code{freq: integer} | ||
#' where T is the item type |
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.
let's fix both
Merged to master |
## What changes were proposed in this pull request? Follow up style fixes to PIC in R; see apache#23072 ## How was this patch tested? Existing tests. Closes apache#23292 from srowen/SPARK-19827.2. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>
## What changes were proposed in this pull request? Follow up style fixes to PIC in R; see apache#23072 ## How was this patch tested? Existing tests. Closes apache#23292 from srowen/SPARK-19827.2. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>
What changes were proposed in this pull request?
Follow up style fixes to PIC in R; see #23072
How was this patch tested?
Existing tests.