Skip to content

[SPARK-14772][ML,PySpark]Python ML Params.copy treats uid, paramMaps … - #12888

Closed
hujy wants to merge 3 commits into
apache:masterfrom
hujy:14772
Closed

[SPARK-14772][ML,PySpark]Python ML Params.copy treats uid, paramMaps …#12888
hujy wants to merge 3 commits into
apache:masterfrom
hujy:14772

Conversation

@hujy

@hujyhujy commented May 4, 2016

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

The patch referenced SPARK-14772
After apply this patch, user can choose to just copy the default param map or the param map.

How was this patch tested?

Unit test, test locally.

return self._defaultParamMap[param]

@since("1.4.0")
def extractParamMap(self, extra=None):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please document this new param

@holdenk

Copy link
Copy Markdown
Contributor

Thanks for tackling this issue :) For a better understanding - is there a reason why adding a flag for this behaviour instead of just changing it (since it is a bug) - do we expect people to want to explicitly copy the default param map in this way?

@holdenk

Copy link
Copy Markdown
Contributor

Also it would be good to have tests to ensure the change has the desired impact.

@hujy

hujy commented May 5, 2016

Copy link
Copy Markdown
ContributorAuthor

@holdenk, I'm checking with @jkbradley about "respect the difference between defaultParamMap and paramMap" in the requirement. The new input parameter is optional and will not impact the user not want to copy default param. : )

@hujy

hujy commented May 13, 2016

Copy link
Copy Markdown
ContributorAuthor

ok with test

@AmplabJenkins

Copy link
Copy Markdown

Can one of the admins verify this patch?

@holdenk

Copy link
Copy Markdown
Contributor

So to trigger the test we will need one of the committers - e.g. @davies is one of the more active Python committers (although he has been busy lately) so we can also check with @MLnick . It would also be nice if you could document the param (even if we don't expect the user to use it - its useful to have it noted for other developers and so users know not to worry about it).

@jkbradley

Copy link
Copy Markdown
Member

@hujy Thank you for sending this PR, and apologies for not seeing it earlier. Since the other PR for this JIRA is ready to merge, could you please close this issue? Thanks again!

@srowensrowen mentioned this pull request Mar 22, 2017
zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025
Closesapache#16819Closesapache#13467Closesapache#16083Closesapache#17135Closesapache#8785Closesapache#16278Closesapache#16997Closesapache#17073Closesapache#17220
Added:
Closesapache#12059Closesapache#12524Closesapache#12888Closesapache#16061
Author: Sean Owen <sowen@cloudera.com>
Closesapache#17386 from srowen/StalePRs.
Sign up for freeto 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.

4 participants

@hujy@holdenk@AmplabJenkins@jkbradley