Skip to content

[SPARK-16445][MLlib][SparkR] Fix @return description for sparkR mlp summary() method - #15015

Closed
keypointt wants to merge 3 commits into
apache:masterfrom
keypointt:SPARK-16445-2
Closed

[SPARK-16445][MLlib][SparkR] Fix @return description for sparkR mlp summary() method#15015
keypointt wants to merge 3 commits into
apache:masterfrom
keypointt:SPARK-16445-2

Conversation

@keypointt

@keypointtkeypointt commented Sep 8, 2016

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Fix summary() method's @return description for spark.mlp

How was this patch tested?

Ran tests locally on my laptop.

@SparkQA

SparkQA commented Sep 8, 2016

Copy link
Copy Markdown

Test build #65110 has finished for PR 15015 at commit 99d8944.

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

@keypointt

keypointt commented Sep 8, 2016

Copy link
Copy Markdown
ContributorAuthor

hi @felixcheung this is just a minor patch to SPARK-16445, where the @return description is not accurate and fixed here. Do you mind have a look here? Thanks a lot :)

felixcheung
felixcheung reviewed Sep 8, 2016
View reviewed changes
Comment threadR/pkg/R/mllib.R
labelCount <- callJMethod(jobj, "labelCount")
layers <- unlist(callJMethod(jobj, "layers"))
weights <- callJMethod(jobj, "weights")
weights <- matrix(weights, nrow = length(weights))

@felixcheungfelixcheungSep 8, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we add a test for this output?

@felixcheung

felixcheung commented Sep 8, 2016

Copy link
Copy Markdown
Member

cool thanks, just one comment

@keypointt

keypointt commented Sep 8, 2016

Copy link
Copy Markdown
ContributorAuthor

There is already a test expect_equal(length(summary$weights), 64)
https://github.com/apache/spark/blob/master/R/pkg/inst/tests/testthat/test_mllib.R#L371

is it enough or we should add more tests?

@felixcheung

felixcheung commented Sep 8, 2016

Copy link
Copy Markdown
Member

since the format of the output is changing it would be great to check more than just the length in test

@keypointt

keypointt commented Sep 8, 2016

Copy link
Copy Markdown
ContributorAuthor

how about adding type check? expect_equal(typeof(summary$weights), "list")

@HyukjinKwon

HyukjinKwon commented Sep 9, 2016•
edited
Loading

Copy link
Copy Markdown
Member

@felixcheung I think this is a irrelevant comment but is there any issue for enabling Windows test for now? It seems it should have run a test for this PR.

@felixcheung

felixcheung commented Sep 9, 2016

Copy link
Copy Markdown
Member

@keypointt let's have a check for the first few values perhaps?
@HyukjinKwon It seems it would be good to start running R tests though I'm not sure actually - I'd defer to @shivaram

@shivaram

shivaram commented Sep 9, 2016

Copy link
Copy Markdown
Contributor

@HyukjinKwon I thought it should have run the tests ? Any ideas why its not getting picked up ? Is there some other set of steps that we need to do ? (I'm now investigating how it works for Apache Thrift)

@shivaram

shivaram commented Sep 9, 2016

Copy link
Copy Markdown
Contributor

I think we need to file an INFRA ticket like https://issues.apache.org/jira/browse/INFRA-11294 -- I'll file one now

@shivaram

shivaram commented Sep 9, 2016

Copy link
Copy Markdown
Contributor

Created https://issues.apache.org/jira/browse/INFRA-12590

@HyukjinKwon

HyukjinKwon commented Sep 9, 2016

Copy link
Copy Markdown
Member

I will check this out as far as I can and be back.

@HyukjinKwon

HyukjinKwon commented Sep 9, 2016

Copy link
Copy Markdown
Member

@dongjoon-hyun Meanwhile, do you mind if I ask whether you have any idea on this maybe?

@dongjoon-hyun

dongjoon-hyun commented Sep 9, 2016

Copy link
Copy Markdown
Member

Up to my knowledge, INFRA ticket made by @shivaram is enough for now. For Apache REEF, we filed the following INFRA issue like that.

https://issues.apache.org/jira/browse/INFRA-11411

@SparkQA

SparkQA commented Sep 9, 2016

Copy link
Copy Markdown

Test build #65135 has finished for PR 15015 at commit 91ae8d6.

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

@felixcheung

felixcheung commented Sep 9, 2016

Copy link
Copy Markdown
Member

LGTM

@SparkQA

SparkQA commented Sep 9, 2016

Copy link
Copy Markdown

Test build #65173 has finished for PR 15015 at commit d526890.

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

@shivaram

shivaram commented Sep 10, 2016

Copy link
Copy Markdown
Contributor

Thanks @keypointt - Merging into master

wgtmac pushed a commit to wgtmac/spark that referenced this pull request Sep 19, 2016
@keypointt@wgtmac
[SPARK-16445][MLLIB][SPARKR] Fix@returndescription for sparkR mlp s…
9e6680e
…ummary() method
## What changes were proposed in this pull request?
Fix summary() method's `return` description for spark.mlp
## How was this patch tested?
Ran tests locally on my laptop.
Author: Xin Ren <iamshrek@126.com>
Closesapache#15015 from keypointt/SPARK-16445-2.
Sign up for freeto 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

@keypointt@SparkQA@felixcheung@HyukjinKwon@shivaram@dongjoon-hyun