Skip to content

Conversation

@harsha2010
Copy link

The base classifier input and output columns are ignored in favor of the ones specified in OneVsRest.

@harsha2010
Copy link
Author

@jkbradley please review. i didn't realize PredictorParams had no setters and setLabelCol, etc were in Predictor which OneVsRest does not extend. This means OneVsRest needs to expose setters for label, features etc that the underlying classifier needs.

@SparkQA
Copy link

SparkQA commented Jun 4, 2015

Test build #34143 has finished for PR 6631 at commit 108d3d7.

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

@feynmanliang
Copy link
Contributor

This LGTM. However, after reading this I feel that OneVsRestModel should extend ClassificationModel. I've created SPARK-8799 for that.

@jkbradley
Copy link
Member

Sorry for the delay! Will review now. Jenkins test this please

@jkbradley
Copy link
Member

I guess there are merge conflicts...but will comment anyways

Copy link
Member

Choose a reason for hiding this comment

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

Is this saying that the base classifier should output "predictionCol"? It probably should send output to a temp col, to be removed.

@jkbradley
Copy link
Member

@harsha2010 We should document clearly that the input and output columns of the base classifier are ignored.

@SparkQA
Copy link

SparkQA commented Jul 2, 2015

Test build #36433 has finished for PR 6631 at commit 108d3d7.

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

@jkbradley
Copy link
Member

Ping : )

@harsha2010
Copy link
Author

oops, thanks for reminding me...will update the merged patch tonight

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #38041 has finished for PR 6631 at commit b7024b1.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Member

Could you please add doc to the "classifier" Param indicating that the base classifier input and output columns are ignored in favor of the ones specified in OneVsRest? Other than that, it looks good.

@SparkQA
Copy link

SparkQA commented Jul 23, 2015

Test build #38178 has finished for PR 6631 at commit 6591dc6.

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

@jkbradley
Copy link
Member

LGTM Would you mind adding a description before we merge this, e.g., to say that the OneVsRest input/output col params override the base classifier ones? Thanks!

@harsha2010
Copy link
Author

@jkbradley , sure, do you mean in the Spark docs? The description is already in the code docs for the params of OneVsRest.

@jkbradley
Copy link
Member

Oh, no, I mean the PR description (the first comment), to become part of the commit message.

@harsha2010 harsha2010 changed the title [Spark-8092][ml] Allow OneVsRest Classifier feature and label column names to be configurable [Spark-8092][ml] Allow OneVsRest Classifier feature and label column names to be configurable. The base classifier input and output columns are ignored in favor of the ones specified in [[OneVsRest]]. Jul 24, 2015
@harsha2010 harsha2010 changed the title [Spark-8092][ml] Allow OneVsRest Classifier feature and label column names to be configurable. The base classifier input and output columns are ignored in favor of the ones specified in [[OneVsRest]]. [Spark-8092][ml] Allow OneVsRest Classifier feature and label column names to be configurable. The base classifier input and output columns are ignored in favor of the ones specified in OneVsRest. Jul 24, 2015
@harsha2010 harsha2010 changed the title [Spark-8092][ml] Allow OneVsRest Classifier feature and label column names to be configurable. The base classifier input and output columns are ignored in favor of the ones specified in OneVsRest. [Spark-8092][ml] Allow OneVsRest Classifier feature and label column names to be configurable. Jul 24, 2015
@harsha2010
Copy link
Author

@jkbradley , thanks., added the documentation to the first comment in this PR

@jkbradley
Copy link
Member

Thanks! I'll merge this with master

@asfgit asfgit closed this in d4d762f Jul 24, 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

Development

Successfully merging this pull request may close these issues.

4 participants