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

[MRG+1] Return nan in RadiusNeighborsRegressor for empty neighbor set #9655

Merged
merged 25 commits into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@abjer
Contributor

abjer commented Aug 30, 2017

Reference Issue

Fixes #9654

What does this implement/fix? Explain your changes.

RadiusNeighborsRegressor is behaving differently when there are no neighbors for a sample between when weights are or aren't used. This PR fixes this inconsistency. This PR also fixes raised error when no available data points for RadiusNeighborRegression using non-uniform weights.

Fix to issue 9654
Fixes problem with raise error when no available data points for RadiusNeighborRegression using non-uniform weights.

@abjer abjer changed the title from [WIP] Fix to issue 9654 to [WIP] Return nan for RadiusNeighborsRegressor with non-uniform weights Aug 30, 2017

@@ -291,9 +291,28 @@ def predict(self, X):
y_pred = np.array([np.mean(_y[ind, :], axis=0)

This comment has been minimized.

@jnothman

jnothman Aug 31, 2017

Member

Do we want this to be issuing a warning in this case? I think the tests should assert either that a warning is raised, or that none is.

@jnothman

jnothman Aug 31, 2017

Member

Do we want this to be issuing a warning in this case? I think the tests should assert either that a warning is raised, or that none is.

This comment has been minimized.

@abjer

abjer Aug 31, 2017

Contributor

I have made a test now that asserts no warning is raised.

@abjer

abjer Aug 31, 2017

Contributor

I have made a test now that asserts no warning is raised.

abjer added some commits Aug 31, 2017

Simplify fix
Used @jnothman suggestion to simplify fix
Show outdated Hide outdated sklearn/neighbors/tests/test_neighbors.py Outdated
Show outdated Hide outdated sklearn/neighbors/regression.py Outdated

abjer added some commits Sep 6, 2017

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Sep 8, 2017

Contributor

@jnothman I think it is ready now for merge - do you agree?

Contributor

abjer commented Sep 8, 2017

@jnothman I think it is ready now for merge - do you agree?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 9, 2017

Member
Member

jnothman commented Sep 9, 2017

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Sep 9, 2017

Contributor

Alright, cheers!

Contributor

abjer commented Sep 9, 2017

Alright, cheers!

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 10, 2017

Member

@jnothman I think it is ready now for merge - do you agree?

You should change the title from WIP to MRG

Member

jnothman commented Sep 10, 2017

@jnothman I think it is ready now for merge - do you agree?

You should change the title from WIP to MRG

@jnothman jnothman changed the title from [WIP] Return nan for RadiusNeighborsRegressor with non-uniform weights to [MRG] Return nan for RadiusNeighborsRegressor with non-uniform weights Sep 10, 2017

abjer added some commits Sep 26, 2017

Empty warning for RadiusNeighborRegression
Added new warning when prediction of RadiusNeighborRegression returns empty values. Also suppressed warning for numpy warnings.

Moreover, added copyright and fixed erroneous description of output.
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 27, 2017

Member

with np.errstate would be preferable

Member

jnothman commented on sklearn/neighbors/regression.py in 891e38b Sep 27, 2017

with np.errstate would be preferable

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 27, 2017

Member

Perhaps use full_like(y[0])

Member

jnothman commented on sklearn/neighbors/regression.py in 891e38b Sep 27, 2017

Perhaps use full_like(y[0])

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 27, 2017

Member

Looking much friendlier, thanks!

Member

jnothman commented on 891e38b Sep 27, 2017

Looking much friendlier, thanks!

@jnothman

Otherwise LGTM, thanks!

Please add an entry to whats_new

abjer added some commits Sep 27, 2017

Notation fix
Fixed more PEP8 and used @jnothman idea of using full_like
Show outdated Hide outdated sklearn/neighbors/regression.py Outdated
Show outdated Hide outdated sklearn/neighbors/regression.py Outdated

@jnothman jnothman changed the title from [MRG] Return nan for RadiusNeighborsRegressor with non-uniform weights to [MRG+1] Return nan for RadiusNeighborsRegressor with non-uniform weights Sep 27, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 27, 2017

Member

LGTM

Member

jnothman commented Sep 27, 2017

LGTM

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Sep 27, 2017

Contributor

Great :)

Contributor

abjer commented Sep 27, 2017

Great :)

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 27, 2017

Member

Please still add an entry in whats_new

Member

jnothman commented Sep 27, 2017

Please still add an entry in whats_new

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Oct 18, 2017

Contributor

@jnothman: good suggestion - I've pushed an update

Contributor

abjer commented Oct 18, 2017

@jnothman: good suggestion - I've pushed an update

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Nov 16, 2017

Contributor

@jnothman hi again joel, is there anything I can do to get a 2nd review? do you have some candidates for whom I should tag?

Contributor

abjer commented Nov 16, 2017

@jnothman hi again joel, is there anything I can do to get a 2nd review? do you have some candidates for whom I should tag?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 16, 2017

Member

This is pretty straightforward, though a more thorough PR description might help a reviewer come on board. Key idea: RadiusNeighborsRegressor is behaving differently when there are no neighbors for a sample between when weights are or aren't used. Perhaps @qinhanmin2014, @lesteve...

Member

jnothman commented Nov 16, 2017

This is pretty straightforward, though a more thorough PR description might help a reviewer come on board. Key idea: RadiusNeighborsRegressor is behaving differently when there are no neighbors for a sample between when weights are or aren't used. Perhaps @qinhanmin2014, @lesteve...

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Nov 16, 2017

Contributor

Thanks a lot Joel - will follow those suggestions

Contributor

abjer commented Nov 16, 2017

Thanks a lot Joel - will follow those suggestions

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Nov 16, 2017

Contributor

Hi @qinhanmin2014 and @lesteve would you mind taking a look at this PR?

Contributor

abjer commented Nov 16, 2017

Hi @qinhanmin2014 and @lesteve would you mind taking a look at this PR?

@qinhanmin2014

Still some concerns before my personal LGTM. Kindly forgive if some of them are naive.
@jnothman would be grateful if you can check these comments.

Show outdated Hide outdated sklearn/neighbors/regression.py Outdated
Show outdated Hide outdated sklearn/neighbors/tests/test_neighbors.py Outdated

abjer added some commits Nov 19, 2017

@qinhanmin2014

Could you please fix the flake8 errors according to Travis log (See the bottom of https://travis-ci.org/scikit-learn/scikit-learn/jobs/304461869)? I think my LGTM is very close :)

Show outdated Hide outdated sklearn/neighbors/regression.py Outdated
Show outdated Hide outdated sklearn/neighbors/tests/test_neighbors.py Outdated
Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated

@qinhanmin2014 qinhanmin2014 changed the title from [MRG+1] Return nan for RadiusNeighborsRegressor with non-uniform weights to [MRG+1] Return nan in RadiusNeighborsRegressor for empty neighbor set Nov 20, 2017

@qinhanmin2014

LGTM ping @jnothman
Latest change since your +1
(1) not block warnings, use similar way as another case instead.
(2)remove redundant tests, only keep two tests for the two cases.

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Nov 20, 2017

Contributor

@qinhanmin2014 thank you so much for the review and very constructive suggestions

Contributor

abjer commented Nov 20, 2017

@qinhanmin2014 thank you so much for the review and very constructive suggestions

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 20, 2017

Member

Great!

Member

jnothman commented Nov 20, 2017

Great!

@jnothman

Apart from what's new being unclear, this is good to merge

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Nov 21, 2017

Member

Apart from what's new being unclear, this is good to merge

Sorry that I somehow miss the what's new in the final check

@abjer I believe current what's new is just a misoperation? The previous what's new seems fine (except for the format I've pointed out). So please correct current what's new accordingly. I'll merge when CIs are green.

Member

qinhanmin2014 commented Nov 21, 2017

Apart from what's new being unclear, this is good to merge

Sorry that I somehow miss the what's new in the final check

@abjer I believe current what's new is just a misoperation? The previous what's new seems fine (except for the format I've pointed out). So please correct current what's new accordingly. I'll merge when CIs are green.

@abjer

This comment has been minimized.

Show comment
Hide comment
@abjer

abjer Nov 21, 2017

Contributor

@jnothman thanks for pointing that out - I have submitted a new version. I removed the mention of duplicate errors as @qinhanmin2014 idea of computation fixed the issue. Thanks both of you for the big support in this PR.

Contributor

abjer commented Nov 21, 2017

@jnothman thanks for pointing that out - I have submitted a new version. I removed the mention of duplicate errors as @qinhanmin2014 idea of computation fixed the issue. Thanks both of you for the big support in this PR.

@jnothman jnothman merged commit 8e1efb0 into scikit-learn:master Nov 21, 2017

5 of 6 checks passed

lgtm analysis: Python Running analyses for revisions
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.13%)
Details
codecov/project 96.13% (+<.01%) compared to bbdcd70
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Nov 21, 2017

Member

Thanks @abjer for your contributions and your patience :)

Member

qinhanmin2014 commented Nov 21, 2017

Thanks @abjer for your contributions and your patience :)

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment