Skip to content
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.

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

[SPARK-10274][MLlib] Add @since annotation to pyspark.mllib.fpm #8665

Closed
wants to merge 4 commits into from

Conversation

yu-iskw
Copy link
Contributor

@yu-iskw yu-iskw commented Sep 9, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Sep 9, 2015

Test build #42191 has finished for PR 8665 at commit babb1ea.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Instance(w: Double, a: Vector, b: Double)
    • class WeibullGenerator(
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):

@mengxr
Copy link
Contributor

mengxr commented Sep 9, 2015

Does @since work on classes?

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Sep 9, 2015

@mengxr unfortunately, a class which inherits from object doesn't work. And as I commented on SPARK-10276, it doesn't work with @classmethod.

# Work
@since("1.6.0")
class Foo():

# Not Work
@since("1.6.0")
class Foo(object):

When I tried to add @since to a class which inherits from object, I got an error. It seems that we can't rewrite __doc__.

Traceback (most recent call last):
  File "tmp.py", line 24, in <module>
    class Foo(object):
  File "tmp.py", line 13, in deco
    f.__doc__ = ".. versionadded:: %s" % (version)
AttributeError: attribute '__doc__' of 'type' objects is not writable

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Sep 9, 2015

It seems that it works well with a class which inherits from object under python 3.3 or later. At least, it doesn't work under python 2.6.8, 2.7, 3.2.

This is a test script for @since. https://gist.github.com/yu-iskw/9f5a702b6657c1887b9b

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Sep 9, 2015

@davies do you have any good idea about the above issue?

@davies
Copy link
Contributor

davies commented Sep 9, 2015

We should add .. addedversion:: 1.4 for classes

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Sep 9, 2015

@davies thank you for the comment. Just to be sure, we should add .. addedversion:: 1.4 to class methods, right?

@davies
Copy link
Contributor

davies commented Sep 9, 2015

I think we should add that to the docstring of class, the class methods already had it, isn't it?

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Sep 10, 2015

Right. I understand @since depends on an order of decorators. @since should be called before @classmethod.

# Work
@classmethod
@since("1.4.0")
def foo(cls):

# Not Work
@since("1.4.0")
@classmethod
def bar(cls):

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Sep 10, 2015

@mengxr could you review it?

@SparkQA
Copy link

SparkQA commented Sep 10, 2015

Test build #42230 has finished for PR 8665 at commit 0e093f1.

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

@noel-smith
Copy link
Contributor

@mengxr @davies Just to confirm, before @yu-iskw and I make the changes - we want to stick with the two-part version numbers (@since(1.4)) used in pyspark.sql instead of the full 3-part numbers (1.4.0) used in the other APIs - correct?

@davies
Copy link
Contributor

davies commented Sep 14, 2015

@noel-smith I prefer two part version numbers (with a few exceptions), that's the way python standard libraries do, but we use three parts in Scala, @mengxr how do you think?

@mengxr
Copy link
Contributor

mengxr commented Sep 14, 2015

We should use version numbers consistently between Python and Scala, and we do have a few APIs added in minor releases (DataFrames, some ML pipeline APIs). I would prefer using a.b.c instead of a.b.

@davies
Copy link
Contributor

davies commented Sep 14, 2015

LGTM (using three parts version in MLlib, we could re-visit the SQL parts later)

@noel-smith
Copy link
Contributor

Sounds good - thanks for confirming - I'll reinstate the thre-part version numbers in my PRs.

@@ -41,8 +41,11 @@ class FPGrowthModel(JavaModelWrapper):
>>> model = FPGrowth.train(rdd, 0.6, 2)
>>> sorted(model.freqItemsets().collect())
[FreqItemset(items=[u'a'], freq=4), FreqItemset(items=[u'c'], freq=3), ...

.. addedversion:: 1.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be versionadded. Try make html under python/docs. Please look for compile warnings and check generated html doc.

@SparkQA
Copy link

SparkQA commented Sep 16, 2015

Test build #42528 has finished for PR 8665 at commit 817b168.

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

@mengxr
Copy link
Contributor

mengxr commented Sep 16, 2015

@yu-iskw Could you also add versionadded to FPGrowth and FreqItemset?

@SparkQA
Copy link

SparkQA commented Sep 17, 2015

Test build #42582 has finished for PR 8665 at commit f1ce652.

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

@asfgit asfgit closed this in c74d38f Sep 17, 2015
@mengxr
Copy link
Contributor

mengxr commented Sep 17, 2015

Merged into master. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants