Skip to content

[SPARK-20764][ML][PySpark][FOLLOWUP]Fix visibility discrepancy with numInstances and degreesOfFreedom in LR and GLR - Python version #18068

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

[SPARK-20764][ML][PySpark][FOLLOWUP]Fix visibility discrepancy with numInstances and degreesOfFreedom in LR and GLR - Python version #18068

wants to merge 4 commits into from

Conversation

@mpjlu
Copy link

@mpjlu mpjlu commented May 23, 2017

What changes were proposed in this pull request?

Add test cases for PR-18062

How was this patch tested?

The existing UT

@mpjlu mpjlu closed this May 23, 2017
@mpjlu mpjlu reopened this May 23, 2017
@SparkQA
Copy link

SparkQA commented May 23, 2017

Test build #77231 has started for PR 18068 at commit 013adc4.

@SparkQA
Copy link

SparkQA commented May 23, 2017

Test build #77229 has finished for PR 18068 at commit 7bbfe3a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class DecisionTreeClassifierWrapperWriter(instance: DecisionTreeClassifierWrapper)
  • class DecisionTreeClassifierWrapperReader extends MLReader[DecisionTreeClassifierWrapper]
  • class DecisionTreeRegressorWrapperWriter(instance: DecisionTreeRegressorWrapper)
  • class DecisionTreeRegressorWrapperReader extends MLReader[DecisionTreeRegressorWrapper]

MLnick
MLnick reviewed May 23, 2017
View reviewed changes
python/pyspark/ml/tests.py
# test evaluation (with training dataset) produces a summary with same values
# one check is enough to verify a summary is returned, Scala version runs full test
# one check is enough to verify a summary is returned
# The child class LinearRegressionTrainingSummary runs full test
Copy link
Contributor

@MLnick MLnick May 23, 2017

Choose a reason for hiding this comment

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

I'm not sure what this comment means?

Copy link
Author

@mpjlu mpjlu May 23, 2017

Choose a reason for hiding this comment

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

The comment is not related with this PR, just because the previous comment is misleading.

Copy link
Contributor

@yanboliang yanboliang May 24, 2017

Choose a reason for hiding this comment

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

@MLnick This is because the type of LinearRegressionModel.summary is LinearRegressionTrainingSummary, but the return type of LinearRegressionModel.evalute() is LinearRegressionSummary. Theoretically we should test both, but we can simplify the test for only check one function since we have checked all functions in the child class. Thanks for @mpjlu to update this comments.

mpjlu
mpjlu commented May 23, 2017
View reviewed changes
python/pyspark/ml/tests.py
# test evaluation (with training dataset) produces a summary with same values
# one check is enough to verify a summary is returned, Scala version runs full test
# one check is enough to verify a summary is returned
# The child class LinearRegressionTrainingSummary runs full test
Copy link
Author

@mpjlu mpjlu May 23, 2017

Choose a reason for hiding this comment

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

I think this is not because Scala version runs full test. Even Scala version runs full test, we still need the function call test.
If a child class have done the function call test, we don't need to test parent class again.

@mpjlu
Copy link
Author

mpjlu commented May 23, 2017

retest this please

@SparkQA
Copy link

SparkQA commented May 23, 2017

Test build #77234 has finished for PR 18068 at commit 013adc4.

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

@yanboliang
Copy link
Contributor

yanboliang commented May 24, 2017

LGTM, merged into master and branch-2.2. Thanks for all.

asfgit pushed a commit that referenced this pull request May 24, 2017
@yanboliang
[SPARK-20764][ML][PYSPARK][FOLLOWUP] Fix visibility discrepancy with …
e936a96 
…numInstances and degreesOfFreedom in LR and GLR - Python version

## What changes were proposed in this pull request?
Add test cases for PR-18062

## How was this patch tested?
The existing UT

Author: Peng <[email protected]>

Closes #18068 from mpjlu/moreTest.

(cherry picked from commit 9afcf12)
Signed-off-by: Yanbo Liang <[email protected]>
@asfgit asfgit closed this in 9afcf12 May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@MLnick MLnick MLnick approved these changes

@yanboliang yanboliang yanboliang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mpjlu @SparkQA @yanboliang @MLnick