Skip to content

Conversation

@BryanCutler
Copy link
Member

What changes were proposed in this pull request?

Fixed the PySpark Params.copy method to behave like the Scala implementation. The main issue was that it did not account for the _defaultParamMap and merged it into the explicitly created param map.

How was this patch tested?

Added new unit test to verify the copy method behaves correctly for copying uid, explicitly created params, and default params.

@BryanCutler
Copy link
Member Author

Ping @holdenk @jkbradley , please take a look when you get a chance - thanks!

@holdenk
Copy link
Contributor

holdenk commented Feb 1, 2017

Let me try and take a look tomorrow - I'm in transit today.

@SparkQA
Copy link

SparkQA commented Feb 1, 2017

Test build #72263 has finished for PR 16772 at commit 8881992.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Feb 2, 2017

Test build #72270 has finished for PR 16772 at commit c25c127.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@BryanCutler BryanCutler force-pushed the pyspark-ml-param_copy-Scala_sync-SPARK-14772 branch from c25c127 to ce59d74 Compare February 2, 2017 01:06
@SparkQA
Copy link

SparkQA commented Feb 2, 2017

Test build #72271 has finished for PR 16772 at commit ce59d74.

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

@BryanCutler
Copy link
Member Author

ping @holdenk - mind taking a look?

@jkbradley
Copy link
Member

LGTM
Merging with master

@BryanCutler would you mind sending a backport PR against branch-2.1 to run Jenkins tests?

Thank you!

@jkbradley
Copy link
Member

well...will merge after new tests

@jkbradley
Copy link
Member

jenkins test this please

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #3581 has finished for PR 16772 at commit ce59d74.

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

@jkbradley
Copy link
Member

OK merging now. @BryanCutler do let me know if you don't have time to send a backport--thanks!

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73383 has finished for PR 16772 at commit ce59d74.

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

@asfgit asfgit closed this in 2f69e3f Feb 24, 2017
@BryanCutler
Copy link
Member Author

BryanCutler commented Feb 24, 2017 via email

Yunni pushed a commit to Yunni/spark that referenced this pull request Feb 27, 2017
…lementation

## What changes were proposed in this pull request?
Fixed the PySpark Params.copy method to behave like the Scala implementation.  The main issue was that it did not account for the _defaultParamMap and merged it into the explicitly created param map.

## How was this patch tested?
Added new unit test to verify the copy method behaves correctly for copying uid, explicitly created params, and default params.

Author: Bryan Cutler <[email protected]>

Closes apache#16772 from BryanCutler/pyspark-ml-param_copy-Scala_sync-SPARK-14772.
@BryanCutler BryanCutler deleted the pyspark-ml-param_copy-Scala_sync-SPARK-14772 branch March 7, 2017 20:01
Sign up for free to 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