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-14926] [ML] OneVsRest labelMetadata uses incorrect name #13762

Conversation

josh-howes
Copy link
Contributor

This contribution is my original work and I license the work to the project under the project's open source license.

What changes were proposed in this pull request?

OneVsRestModel applies labelMetadata to the output column, but the metadata could contain the wrong name. The attribute name was modified to match predictionCol.

How was this patch tested?

Manual Tests

@mengxr
Copy link
Contributor

mengxr commented Jun 21, 2016

@josh-howes Did you try compiling the code? predictionCol is not a Metadata instance.

@jkbradley
Copy link
Member

Ping

@SparkQA
Copy link

SparkQA commented Sep 6, 2016

Test build #3251 has finished for PR 13762 at commit 22a3a04.

  • This patch fails to build.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@josh-howes josh-howes force-pushed the Bugfix/SPARK-14926-One_vs_Rest-labelMetadata branch from bd510ec to b4badae Compare September 8, 2016 10:05
@josh-howes
Copy link
Contributor Author

@SparkQA build

@srowen
Copy link
Member

srowen commented Sep 12, 2016

I think we should close this PR.

@josh-howes
Copy link
Contributor Author

I recently pushed a change to this but @SparkQA didn't kick off an automated build. Is that because I squashed my commits?

@srowen
Copy link
Member

srowen commented Sep 12, 2016

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #65256 has finished for PR 13762 at commit b4badae.

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

Copy link
Member

@jkbradley jkbradley left a comment

Choose a reason for hiding this comment

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

Also, could you please add a line to a unit test to verify that the predictionCol has the correct metadata? Thank you!

@@ -196,8 +196,13 @@ final class OneVsRestModel private[ml] (
}

// output label and label metadata as prediction
val predictionMetadata = new MetadataBuilder()
.withMetadata(labelMetadata)
.putString("name", predictionCol.name)
Copy link
Member

Choose a reason for hiding this comment

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

This should go through the standard ML attribute constructors to modify labelMetadata:

val predictionMetadata = Attribute.fromMetadata(labelMetadata)
  .withName($(predictionCol))
  .toMetadata()

@SparkQA
Copy link

SparkQA commented Oct 7, 2016

Test build #3299 has finished for PR 13762 at commit b4badae.

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

@HyukjinKwon
Copy link
Member

Hi @josh-howes is it still active? If so, please address the comments above. Otherwise, I would rather like to propose to close this.

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