Skip to content

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Jun 3, 2016

What changes were proposed in this pull request?

Change FPGrowth from private to private[spark]. If no numPartitions is specified, then default value -1 is used. But -1 is only valid in the construction function of FPGrowth, but not in setNumPartitions. So I make this change and use the constructor directly rather than using set method.

How was this patch tested?

Unit test is added

@SparkQA
Copy link

SparkQA commented Jun 3, 2016

Test build #59923 has finished for PR 13493 at commit 56cd7cf.

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

@MechCoder
Copy link
Contributor

lgtm cc: @MLnick

@mengxr
Copy link
Contributor

mengxr commented Jun 29, 2016

Shall we change the logic in the Python wrapper and set numPartitions correctly if it is -1? Please also update the PR description to add more details to the changes.

@SparkQA
Copy link

SparkQA commented Aug 30, 2016

Test build #3236 has finished for PR 13493 at commit 56cd7cf.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Member

Pinging! Is this still active?

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 7, 2016

@jkbradley I will update the PR soon

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 8, 2016

@mengxr @jkbradley I think we should keep as less logic in python side as possible. So I didn't update the PR, please help review. Thanks

@SparkQA
Copy link

SparkQA commented Sep 8, 2016

Test build #65071 has finished for PR 13493 at commit f0d37c8.

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

@holdenk
Copy link
Contributor

holdenk commented Sep 14, 2016

This looks pretty reasonable to me, the only thing is (and its relatively minor) testing the default param might be better as a tests.py test rather than as a doc test since the doctests are part of the documentation and we might not need this in the docs along with the current example.

@jkbradley
Copy link
Member

Same here, looks fine except for @holdenk 's comment. Thanks!

@jkbradley
Copy link
Member

Ping --- let me know if you'd like someone to take it over. Thanks!

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 6, 2016

Sorry, I am in vacation. Will update it next week.

@holdenk
Copy link
Contributor

holdenk commented Oct 6, 2016

@zjffdu Enjoy vacation and no worries - I don't think this is blocking anything and we can circle back in a week :)

@SparkQA
Copy link

SparkQA commented Oct 9, 2016

Test build #66589 has finished for PR 13493 at commit e8fefc0.

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

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 9, 2016

PR is updated, @holdenk @jkbradley

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 17, 2016

ping @jkbradley @holdenk

@SparkQA
Copy link

SparkQA commented Aug 24, 2017

Test build #81076 has finished for PR 13493 at commit e8fefc0.

  • This patch fails PySpark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@WeichenXu123
Copy link
Contributor

ping @zjffdu Looks reasonable fix, But pls resolve the conflicts! Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is useless.

@SparkQA
Copy link

SparkQA commented Sep 29, 2017

Test build #82315 has finished for PR 13493 at commit 3575444.

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

@SparkQA
Copy link

SparkQA commented Sep 29, 2017

Test build #82319 has finished for PR 13493 at commit fbbcd26.

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

@gatorsmile
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Sep 29, 2017

Test build #82330 has finished for PR 13493 at commit fbbcd26.

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

@lu-wang-dl
Copy link
Contributor

The bug is confirmed. The fix looks pretty reasonable to me. ping @jkbradley .

@jkbradley
Copy link
Member

LGTM pending fresh tests
Sorry for the delay!

@SparkQA
Copy link

SparkQA commented May 1, 2018

Test build #4167 has finished for PR 13493 at commit fbbcd26.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@zjffdu
Copy link
Contributor Author

zjffdu commented May 2, 2018

Thanks @jkbradley The failed tests seems unrelated.

@lu-wang-dl
Copy link
Contributor

LGTM retest this please

@SparkQA
Copy link

SparkQA commented May 3, 2018

Test build #90147 has finished for PR 13493 at commit fbbcd26.

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

@WeichenXu123
Copy link
Contributor

LGTM!

@jkbradley
Copy link
Member

jkbradley commented May 7, 2018

Merging with master
Thanks all!

@asfgit asfgit closed this in 56a52e0 May 7, 2018
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.

9 participants