8000 Handling out-of-bounds in IsotonicRegression (clean PR #3147) by mjbommar · Pull Request #3199 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Handling out-of-bounds in IsotonicRegression (clean PR #3147) #3199

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 26, 2014
Merged

Handling out-of-bounds in IsotonicRegression (clean PR #3147) #3199

merged 7 commits into from
May 26, 2014

Conversation

mjbommar
Copy link
Contributor

Here's a clean, follow-up PR based on the other Isotonic merges today and conversation from PR #3147.

  • Added argument out_of_bounds
  • Added test for each value case below
  • Changed default behavior from "raise", i.e., ValueError, to "nan"

Argument behavior

  • "nan": x-values not in training domain -> y-values = NaN
  • "clip": x-values not in training domain -> y-values = y-value for nearest training x
  • "raise": x-values not in training domain -> ValueError exception

Let me know if you want to see L296-310 differently. I'd like to clean up fit vs. fit_transform and some of the matrix/vector notation mismatch in the module anyway.

(My interpretation is also that there is no need for f to be created anew on each call to transform.)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 2629fc7 on mjbommar:isotonic-out-of-bounds-2 into 6a7786a on scikit-learn:master.

@agramfort
Copy link
Member

nice !

+1 for merge

jnothman
jnothman reviewed May 26, 2014
View reviewed changes
sklearn/isotonic.py
bounds_error=False)

# Clip out-of-bounds values if requested.
if self.out_of_bounds == "clip":
Copy link
Member

Choose a reason for hiding this comment

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

This is alright, but seems a little strange to not be handled as an elif in the clause above, seeing as it's mutually exclusive from raise and nan. In this context, I think it's fine to reuse the name T for the clipped data.

@jnothman
Copy link
Member

Mostly looks fine, but I think you should structure that transform with a four-way if.

@NelleV
Copy link
Member
NelleV commented May 26, 2014

LGTM 👍

@GaelVaroquaux
Copy link
Member

LGTM too. Merging, as @NelleV has given her 👍 Thanks, @mjbommar !

GaelVaroquaux added a commit that referenced this pull request May 26, 2014
@GaelVaroquaux
Merge pull request #3199 from mjbommar/isotonic-out-of-bounds-2
d0f6052 
Handling out-of-bounds in IsotonicRegression (clean PR #3147)
@GaelVaroquaux GaelVaroquaux merged commit d0f6052 into scikit-learn:master May 26, 2014
@mjbommar mjbommar mentioned this pull request May 26, 2014
Closed
@amueller amueller mentioned this pull request Jan 29, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mjbommar @coveralls @agramfort @jnothman @NelleV @GaelVaroquaux
0