Skip to content

[SPARK-8151] [MLLIB] pipeline components should correctly implement copy #6622

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 9 commits into from
Closed

[SPARK-8151] [MLLIB] pipeline components should correctly implement copy #6622

wants to merge 9 commits into from

Conversation

@mengxr
Copy link
Contributor

@mengxr mengxr commented Jun 3, 2015

Otherwise, extra params get ignored in PipelineModel.transform. @jkbradley

jkbradley
jkbradley reviewed Jun 3, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala Outdated
Copy link
Member

@jkbradley jkbradley Jun 3, 2015

Choose a reason for hiding this comment

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

When writing copy for CrossValidator, I recall we decided not to copy the extra params to the estimator param.
This case seems analogous, so we probably should not copy the extra params to classifier here.
(I could imagine arguments for either behavior, but we should be consistent.)

Copy link
Member

@jkbradley jkbradley Jun 3, 2015

Choose a reason for hiding this comment

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

After discussion: We will override copy() for CrossValidator too!

@SparkQA
Copy link

SparkQA commented Jun 3, 2015

Test build #34115 has finished for PR 6622 at commit 1dfe3bd.

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

@mengxr mengxr changed the title [SPARK-8087] [MLLIB] PipelineModel.copy should copy stages [WIP] [SPARK-8087] [MLLIB] PipelineModel.copy should copy stages Jun 5, 2015
@mengxr
Copy link
Contributor Author

mengxr commented Jun 5, 2015

Changed the title to WIP because this PR may become relatively large.

@mengxr mengxr changed the title [WIP] [SPARK-8087] [MLLIB] PipelineModel.copy should copy stages [WIP] [SPARK-8151] [MLLIB] pipeline components should correctly implement copy Jun 8, 2015
@mengxr
Copy link
Contributor Author

mengxr commented Jun 8, 2015

Implemented copy for every pipeline component. Let Jenkins run for mima checks.

@SparkQA
Copy link

SparkQA commented Jun 8, 2015

Test build #34402 has finished for PR 6622 at commit d6f7891.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 8, 2015

Test build #34467 has finished for PR 6622 at commit b85b57e.

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

jkbradley
jkbradley reviewed Jun 9, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/feature/IDF.scala
Copy link
Member

@jkbradley jkbradley Jun 9, 2015

Choose a reason for hiding this comment

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

Why does IDFModel need this? It does not use minDocFreq.

@jkbradley
Copy link
Member

jkbradley commented Jun 9, 2015

a few new comments + the old, but it looks good

@mengxr mengxr changed the title [WIP] [SPARK-8151] [MLLIB] pipeline components should correctly implement copy [SPARK-8151] [MLLIB] pipeline components should correctly implement copy Jun 18, 2015
@SparkQA
Copy link

SparkQA commented Jun 18, 2015

Test build #35190 has finished for PR 6622 at commit 26fc1f0.

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

@jkbradley
Copy link
Member

jkbradley commented Jun 19, 2015

LGTM except for the style tests

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #35234 has finished for PR 6622 at commit 0e4c8c4.

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

@asfgit asfgit closed this in 43c7ec6 Jun 19, 2015
@mengxr
Copy link
Contributor Author

mengxr commented Jun 19, 2015

Merged into master and branch-1.4.

asfgit pushed a commit that referenced this pull request Jun 19, 2015
@mengxr
[SPARK-8151] [MLLIB] pipeline components should correctly implement copy
1f2dafb 
Otherwise, extra params get ignored in `PipelineModel.transform`. jkbradley

Author: Xiangrui Meng <[email protected]>

Closes #6622 from mengxr/SPARK-8087 and squashes the following commits:

0e4c8c4 [Xiangrui Meng] fix merge issues
26fc1f0 [Xiangrui Meng] address comments
e607a04 [Xiangrui Meng] merge master
b85b57e [Xiangrui Meng] fix examples/compile
d6f7891 [Xiangrui Meng] rename defaultCopyWithParams to defaultCopy
84ec278 [Xiangrui Meng] remove setter checks due to generics
2cf2ed0 [Xiangrui Meng] snapshot
291814f [Xiangrui Meng] OneVsRest.copy
1dfe3bd [Xiangrui Meng] PipelineModel.copy should copy stages

(cherry picked from commit 43c7ec6)
Signed-off-by: Xiangrui Meng <[email protected]>
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 22, 2015
@mengxr @nemccarthy
[SPARK-8151] [MLLIB] pipeline components should correctly implement copy
d31ecad 
Otherwise, extra params get ignored in `PipelineModel.transform`. jkbradley

Author: Xiangrui Meng <[email protected]>

Closes apache#6622 from mengxr/SPARK-8087 and squashes the following commits:

0e4c8c4 [Xiangrui Meng] fix merge issues
26fc1f0 [Xiangrui Meng] address comments
e607a04 [Xiangrui Meng] merge master
b85b57e [Xiangrui Meng] fix examples/compile
d6f7891 [Xiangrui Meng] rename defaultCopyWithParams to defaultCopy
84ec278 [Xiangrui Meng] remove setter checks due to generics
2cf2ed0 [Xiangrui Meng] snapshot
291814f [Xiangrui Meng] OneVsRest.copy
1dfe3bd [Xiangrui Meng] PipelineModel.copy should copy stages

(cherry picked from commit 43c7ec6)
Signed-off-by: Xiangrui Meng <[email protected]>
Sign up for free to 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.

3 participants

@mengxr @SparkQA @jkbradley