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-10273] Add @since annotation to pyspark.mllib.feature #8633

Closed

Conversation

noel-smith
Copy link
Contributor

Duplicated the @SInCE decorator from pyspark.sql into pyspark (also tweaked to handle functions without docstrings).

Added @SInCE to methods + "versionadded::" to classes (derived from the git file history in pyspark).

@@ -51,6 +51,26 @@
# for back compatibility
from pyspark.sql import SQLContext, HiveContext, SchemaRDD, Row


def since(version):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should belong to SPARK-10373. We should have a separate PR for it and then update this PR. ping @davies

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - no problem - I'll update this (+ SPARK-10269/10271/10272) once SPARK-10373 is complete.

@mengxr
Copy link
Contributor

mengxr commented Sep 8, 2015

add to whitelist

@SparkQA
Copy link

SparkQA commented Sep 8, 2015

Test build #42151 has finished for PR 8633 at commit 3dadc03.

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

@SparkQA
Copy link

SparkQA commented Sep 9, 2015

Test build #42214 has finished for PR 8633 at commit 557aaac.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class DefaultSource extends RelationProvider with DataSourceRegister

@noel-smith noel-smith force-pushed the SPARK-10273-since-mllib-feature branch from 557aaac to e6a4c47 Compare September 9, 2015 19:55
@@ -84,11 +84,14 @@ class Normalizer(VectorTransformer):
>>> nor2 = Normalizer(float("inf"))
>>> nor2.transform(v)
DenseVector([0.0, 0.5, 1.0])

.. versionadded:: 1.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we won't introduce new API in the micro releases, I think it's better to use 1.2 for short. cc @mengxr

Also we could just use float number as version, see pyspark/sql as examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think matching the overall project versioning scheme make it's clearer - but I'm happy to implement it either way.

One thing to watch for with using floats is that you can't differentiate between 1.1 and 1.10 (but it looks like that's unlikely to be a problem from the versioning history).

Copy link
Contributor

Choose a reason for hiding this comment

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

For 1.10 or 1.1.1, we still need to use string. This change is not necessary, just one minor comment.

@SparkQA
Copy link

SparkQA commented Sep 9, 2015

Test build #42215 has finished for PR 8633 at commit e6a4c47.

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

@SparkQA
Copy link

SparkQA commented Sep 10, 2015

Test build #42292 has finished for PR 8633 at commit bcf77a3.

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

@mengxr
Copy link
Contributor

mengxr commented Sep 14, 2015

@noel-smith @yu-iskw Could you help review each other's PRs on pyspark.ml @since versions?

I want to merge them as soon as possible to avoid future merge conflicts. It would be nice if we can distribute the work. Please give LGTM if the PR looks good to you. I will merge them after. Thanks!

@noel-smith
Copy link
Contributor Author

@mengxr @yu-iskw - Sounds like a plan - I'll take a look.

@SparkQA
Copy link

SparkQA commented Sep 14, 2015

Test build #42443 has finished for PR 8633 at commit 7853215.

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

@mengxr
Copy link
Contributor

mengxr commented Sep 15, 2015

LGTM. Merged into master. Thanks!

@asfgit asfgit closed this in 610971e Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants