Skip to content

Conversation

@BenFradet
Copy link
Contributor

Documentation regarding the IndexToString label transformer with code snippets in Scala/Java/Python.

@SparkQA
Copy link

SparkQA commented Dec 6, 2015

Test build #47245 has finished for PR 10166 at commit 10ba98a.

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

@holdenk
Copy link
Contributor

holdenk commented Dec 7, 2015

It might be useful to also document the different ways "missing" labels can be handled - what are your thoughts?

@BenFradet
Copy link
Contributor Author

Hey @holdenk, thanks for reviewing.

Do you mean regarding StringIndexer#setHandleInvalid method? If so, yes that'd be a good addition.

However, I'm not sure if I should include it in this jira/pr or create another, input welcome.

@BenFradet
Copy link
Contributor Author

cc @jkbradley

@holdenk
Copy link
Contributor

holdenk commented Dec 7, 2015

That is what I was referring to, handling it in a follow up JIRA/PR seems ok too (just since one of the things blocking the original implementation was wanting to have it be user controllable if we allowed people to specify their own maps it seemed like good for that to also make it through to the docs).

@jkbradley
Copy link
Member

Thanks for the PR; I'll take a look now!
@holdenk handleInvalid should be in a separate PR since it's for StringIndexer, but I agree it'd be nice to add to the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind moving these to examples/ and pulling the code snippets into here using the include_example functionality? You can find examples of include_example in this .md file. This makes the examples easier to test & maintain.

@jkbradley
Copy link
Member

Those are my only comments; the examples look good. Btw, it's OK this time, but in general, I'd recommend doing little cleanups in a separate PR. Especially when lots of docs are being merged, it's really easy to hit merge conflicts. Thanks! I'll watch for updates.

@BenFradet
Copy link
Contributor Author

@jkbradley Thanks for reviewing, will take those comments into account.

@jkbradley
Copy link
Member

That was a spurious test failure; I asked it to retest

@SparkQA
Copy link

SparkQA commented Dec 8, 2015

Test build #2184 has finished for PR 10166 at commit cb33653.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * public class JavaIndexToStringExample\n

@jkbradley
Copy link
Member

dev/lint-python should catch these issues

@jkbradley
Copy link
Member

LGTM except for the Python style issue

@SparkQA
Copy link

SparkQA commented Dec 8, 2015

Test build #47361 has finished for PR 10166 at commit 9591007.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * public class JavaIndexToStringExample\n

@SparkQA
Copy link

SparkQA commented Dec 8, 2015

Test build #47363 has finished for PR 10166 at commit 9398743.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * public class JavaIndexToStringExample\n

@jkbradley
Copy link
Member

Merging with master and branch-1.6

Thanks for the PR!

asfgit pushed a commit that referenced this pull request Dec 8, 2015
Documentation regarding the `IndexToString` label transformer with code snippets in Scala/Java/Python.

Author: BenFradet <benjamin.fradet@gmail.com>

Closes #10166 from BenFradet/SPARK-12159.

(cherry picked from commit 06746b3)
Signed-off-by: Joseph K. Bradley <joseph@databricks.com>
@asfgit asfgit closed this in 06746b3 Dec 8, 2015
@BenFradet
Copy link
Contributor Author

@jkbradley Should I log a jira for completing the user guide on StringIndexer regarding the handling of missing labels @holdenk was talking about?

@jkbradley
Copy link
Member

If you wouldn't mind, that'd be great, thanks!

@BenFradet
Copy link
Contributor Author

will do

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.

4 participants