Skip to content

Conversation

@BryanCutler
Copy link
Member

What changes were proposed in this pull request?

I reviewed Scala and Python APIs for ml.feature and corrected discrepancies.

How was this patch tested?

Built docs locally, ran style checks

@BryanCutler
Copy link
Member Author

cc @MLnick @holdenk

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58728 has finished for PR 13159 at commit a124a1a.

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

/**
* :: Experimental ::
* Model fitted by [[PCA]].
* Model fitted by [[PCA]]. Transforms vectors to a lower dimensional space.
Copy link
Member

Choose a reason for hiding this comment

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

(Trivial.. but maybe single space between * Model fitted by [[PCA]] and Transforms vectors to a lower dimensional space.)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah you're right, I should have used single space. Thanks @HyukjinKwon !

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58792 has finished for PR 13159 at commit 2b657c5.

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

@BryanCutler
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58795 has finished for PR 13159 at commit 2b657c5.

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

@holdenk
Copy link
Contributor

holdenk commented May 18, 2016

Thanks for doing this, looks like a good improvement. While we are updating the PyDocs here it seems like some of stages that require fitting make links to the transformer/model when they say fitted by and some don't - we could consider fixing the links while we are here but its probably not all that bad to leave as is.

@BryanCutler
Copy link
Member Author

Ya, that would be good to fix and might as well do it here

/**
* :: Experimental ::
* PCA trains a model to project vectors to a low-dimensional space using PCA.
* PCA trains a model to project vectors to a lower dimensional space of the top [[PCA!.k]]
Copy link
Member Author

Choose a reason for hiding this comment

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

while you're looking @holdenk , is this right with the "!"? I saw this somewhere else and without that it seems like it can't find k, but I couldn't find anything about it in scaladoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yah I took a look - it seems to generate the correct link and we use it elsewhere but it doesn't seem to really do much.

@SparkQA
Copy link

SparkQA commented May 18, 2016

Test build #58812 has finished for PR 13159 at commit 09f3078.

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

@MLnick
Copy link
Contributor

MLnick commented May 19, 2016

LGTM, thanks @BryanCutler. Merged to master/branch-2.0

@asfgit asfgit closed this in b1bc5eb May 19, 2016
@rxin
Copy link
Contributor

rxin commented May 20, 2016

@MLnick did you actually merge this in 2.0?

asfgit pushed a commit that referenced this pull request May 20, 2016
I reviewed Scala and Python APIs for ml.feature and corrected discrepancies.

Built docs locally, ran style checks

Author: Bryan Cutler <[email protected]>

Closes #13159 from BryanCutler/ml.feature-api-sync.

(cherry picked from commit b1bc5eb)
Signed-off-by: Reynold Xin <[email protected]>
@MLnick
Copy link
Contributor

MLnick commented May 20, 2016

@rxin I thought I had but must have missed it by mistake. Thanks for picking to 2.0

@BryanCutler BryanCutler deleted the ml.feature-api-sync branch December 2, 2016 00:59
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.

6 participants