Skip to content

[SPARK-23162][PySpark][ML] Add r2adj into Python API in LinearRegressionSummary - #20842

Closed
kevinyu98 wants to merge 3 commits into
apache:masterfrom
kevinyu98:spark-23162
Closed

[SPARK-23162][PySpark][ML] Add r2adj into Python API in LinearRegressionSummary#20842
kevinyu98 wants to merge 3 commits into
apache:masterfrom
kevinyu98:spark-23162

Conversation

@kevinyu98

@kevinyu98kevinyu98 commented Mar 16, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Adding r2adj in LinearRegressionSummary for Python API.

How was this patch tested?

Added unit tests to exercise the api calls for the summary classes in tests.py.

@BryanCutler

BryanCutler commented Mar 21, 2018

Copy link
Copy Markdown
Member

ok to test

@SparkQA

SparkQA commented Mar 21, 2018

Copy link
Copy Markdown

Test build #88492 has finished for PR 20842 at commit 12d18b5.

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

BryanCutler
BryanCutler requested changes Mar 22, 2018
View reviewed changes

@BryanCutlerBryanCutler left a comment

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.

Thanks @kevinyu98! Looks mostly good, just some small doc formatting issues

Comment threadpython/pyspark/ml/regression.py Outdated
@since("2.4.0")
def r2adj(self):
"""
Returns Adjusted R^2^, the adjusted coefficient of determination.

@BryanCutlerBryanCutlerMar 22, 2018•
edited
Loading

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.

R^2^ is a scaladoc format. How about just R^2? and could you fix in def r2 also

(R^2 would mean XOR in python, so R**2 seems the best to me unless someone knows a better format to use with Sphinx)
edit again, let's just stick with R^2, I think it's fine here and no one would think it's XOR

@kevinyu98kevinyu98Mar 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

sure, I will change.

Comment threadpython/pyspark/ml/regression.py Outdated
Returns Adjusted R^2^, the adjusted coefficient of determination.

.. seealso:: `Wikipedia coefficient of determination \
<https://en.wikipedia.org/wiki/Coefficient_of_determination>`

@BryanCutlerBryanCutlerMar 22, 2018

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.

use the same link from the scaladoc https://en.wikipedia.org/wiki/Coefficient_of_determination#Adjusted_R2

It also doesn't generate properly, you need a trailing _ I believe (also need to fix def r2 if you don't mind).

it should be:

.. seealso:: `Wikipedia coefficient of determination \
<http://en.wikipedia.org/wiki/Coefficient_of_determination#Adjusted_R2>`_

If you are able to, building the docs to check these is a good idea

@kevinyu98kevinyu98Mar 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok, done.

@SparkQA

SparkQA commented Mar 24, 2018

Copy link
Copy Markdown

Test build #88556 has finished for PR 20842 at commit ab0b04d.

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

@tengpeng

tengpeng commented Mar 25, 2018

Copy link
Copy Markdown
Contributor

Looks good! Thanks!

@kevinyu98

kevinyu98 commented Mar 26, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@tengpeng Thanks a lot.Not sure the Python style test fail is related to my latest code change in regression.py or not.
I am using this command to run the python style tests locally by using ./dev/lint-python, but got lots of warning from the python docs folder:
Warning, treated as error:
/Users/qianyangyu/IdeaProjects/spark/python/docs/pyspark.rst:18: (WARNING/2) autodoc: failed to import module u'pyspark'; the following exception was raised:
Traceback (most recent call last):
File "/usr/local/Cellar/sphinx-doc/1.6.3_1/libexec/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 657, in import_object
import(self.modname)
ImportError: No module named pyspark
make: *** [html] Error 1
...

BryanCutler
BryanCutler reviewed Mar 26, 2018
View reviewed changes
Comment threadpython/pyspark/ml/regression.py Outdated
"""
Returns Adjusted R^2, the adjusted coefficient of determination.

.. seealso:: `Wikipedia coefficient of determination \

@BryanCutlerBryanCutlerMar 26, 2018

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.

It looks like the error was because the link name is the same as above, maybe try Wikipedia coefficient of determination, Adjusted R^2

@kevinyu98kevinyu98Mar 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok, thanks.

@SparkQA

SparkQA commented Mar 26, 2018

Copy link
Copy Markdown

Test build #88597 has finished for PR 20842 at commit 596f58a.

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

BryanCutler
BryanCutler approved these changes Mar 26, 2018
View reviewed changes

@BryanCutlerBryanCutler left a comment

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.

LGTM

@BryanCutler

BryanCutler commented Mar 26, 2018

Copy link
Copy Markdown
Member

merged to master, thanks @kevinyu98 !

mshtelma pushed a commit to mshtelma/spark that referenced this pull request Apr 5, 2018
@kevinyu98Mykhailo Shtelma
[SPARK-23162][PYSPARK][ML] Add r2adj into Python API in LinearRegress…
d49c6dd
…ionSummary
## What changes were proposed in this pull request?
Adding r2adj in LinearRegressionSummary for Python API.
## How was this patch tested?
Added unit tests to exercise the api calls for the summary classes in tests.py.
Author: Kevin Yu <qyu@us.ibm.com>
Closesapache#20842 from kevinyu98/spark-23162.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BryanCutlerBryanCutlerBryanCutler approved these changes

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

@kevinyu98@BryanCutler@SparkQA@tengpeng