Skip to content

Conversation

@badriub
Copy link

@badriub badriub commented Jul 24, 2015

  1. I've declared a new method predictPointWithProbability in GeneralizedLinearModel and implemented the method only for LogisticRegressionModel.
  2. I calculate the probability using the odds ratio.
  3. For the other implementations of GeneralizedLinearModel, I have added an "not implemented exception". I can look into how to calculate probability if needed.

WRT OVR,
I've extracted the rawProbability as the maxProbability of all classes and added a new column in the returned dataframe.
I've fixed the test to include the new column "rawProbability"

@badriub badriub changed the title Added predictive probability to OneVsRestModel and LogisticRegressionModel [SPARK-9312] [MLLIB] Added predictive probability to OneVsRestModel and LogisticRegressionModel Jul 24, 2015
Copy link
Member

Choose a reason for hiding this comment

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

Without commenting on the suitability of the PR -- I can at least say that throw new Exception is never a great idea. Use UnsupportedOperationException here, for example.

@jkbradley
Copy link
Member

@badriub I just added some thoughts on the JIRA. Could you please take a look and respond there? Thanks!

@badriub
Copy link
Author

badriub commented Jul 28, 2015

I have replied on the JIRA, please take a look. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Used the Intellj importer plugin as recommended on the style guide.

Copy link
Member

Choose a reason for hiding this comment

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

You may need to adjust the import organizer settings, though. The order should be Java, Scala, other, Spark.

@badriub badriub changed the title [SPARK-9312] [MLLIB] Added predictive probability to OneVsRestModel and LogisticRegressionModel [SPARK-9312] [MLLIB] Added confidence factor to OneVsRestModel Jul 29, 2015
@badriub
Copy link
Author

badriub commented Jul 29, 2015

@jkbradley : I've reverted the changes for LogisticRegression (for entire class hierarchy) since they were not needed. I've updated JIRA and PR as discussed.
@srowen : I've used the importer plugin and made sure I follow the style guide.

Copy link
Member

Choose a reason for hiding this comment

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

It will be easier for you if you have OneVsRestParams extend ClassifierParams. That will include the rawPredictionCol, plus the updated validateAndTransformSchema method.

@jkbradley
Copy link
Member

That should do it for a first pass. I'll check back for updates. Thanks!
Also, can you please update:

  • the PR description (the first comment in this PR) to match the more recent changes, and
  • the title to use the tag "ml" instead of "mllib"?

@badriub badriub changed the title [SPARK-9312] [MLLIB] Added confidence factor to OneVsRestModel [SPARK-9312] [ML] Added max confidence factor to OneVsRestModel Aug 12, 2015
@viktortnk
Copy link

@jkbradley @badriub Any updates on this issue?

From diff I see that rawPrediction has Double type. Does it make sense to return an Array of values from each binary classifier?

I'm happy to help if required to get it through

@AxenGitHub
Copy link

Is there any news on this branch? we would benefit a lot from this feature.

@jkbradley
Copy link
Member

Hi, sorry I let this PR get stale. This should be resolved now by #21044 so would you mind closing this issue @badriub ? Thanks though!

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@srowen srowen closed this Dec 16, 2018
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