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+2] make_circles() now works with odd number of samples, test added #10045

Merged
merged 14 commits into from Nov 11, 2017

Conversation

Projects
None yet
7 participants
@christianbraune79
Contributor

christianbraune79 commented Oct 31, 2017

Reference Issues/PRs

Fixes #10037 and adds corresponding tests

What does this implement/fix? Explain your changes.

Fixes he faulty behaviour of make_circles when n_samples is an odd number.
Adds test test_make_circles to datasets/tests/test_samples_generator.py.

Any other comments?

Christian Braune added some commits Oct 29, 2017

@TomDLT

TomDLT approved these changes Oct 31, 2017

LGTM

Show outdated Hide outdated sklearn/datasets/samples_generator.py Outdated
@qinhanmin2014

Please fix the PEP8 error to pass the test :)

Show outdated Hide outdated sklearn/datasets/samples_generator.py Outdated

Christian Braune added some commits Oct 31, 2017

@amueller

you could add another test, but looks good either way.

Show outdated Hide outdated sklearn/datasets/samples_generator.py Outdated
outer_circ_y = np.sin(linspace)
inner_circ_x = outer_circ_x * factor
inner_circ_y = outer_circ_y * factor
# so as not to have the first point = last point, we set endpoint=False

This comment has been minimized.

@amueller

amueller Oct 31, 2017

Member

You're so much smarter than me....

@amueller

amueller Oct 31, 2017

Member

You're so much smarter than me....

Show outdated Hide outdated sklearn/datasets/tests/test_samples_generator.py Outdated

@amueller amueller changed the title from [MRG] make_circles() now works with odd number of samples, test added to [MRG + 1] make_circles() now works with odd number of samples, test added Oct 31, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 1, 2017

Member

Please add an entry to the change log at doc/whats_new.

Member

jnothman commented Nov 1, 2017

Please add an entry to the change log at doc/whats_new.

Christian Braune
Adjusted documentation for make_circles
Added a test to check if really only factors in (0, 1) (excluding borders) are accepted
Adjusted factor check (1.0 was accepted before, though doc said otherwise)
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 3, 2017

Codecov Report

Merging #10045 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10045      +/-   ##
==========================================
+ Coverage   96.19%   96.19%   +<.01%     
==========================================
  Files         336      336              
  Lines       62725    62743      +18     
==========================================
+ Hits        60336    60354      +18     
  Misses       2389     2389
Impacted Files Coverage Δ
sklearn/datasets/tests/test_samples_generator.py 100% <100%> (ø) ⬆️
sklearn/datasets/samples_generator.py 93.42% <100%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9172a59...8a86067. Read the comment docs.

codecov bot commented Nov 3, 2017

Codecov Report

Merging #10045 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10045      +/-   ##
==========================================
+ Coverage   96.19%   96.19%   +<.01%     
==========================================
  Files         336      336              
  Lines       62725    62743      +18     
==========================================
+ Hits        60336    60354      +18     
  Misses       2389     2389
Impacted Files Coverage Δ
sklearn/datasets/tests/test_samples_generator.py 100% <100%> (ø) ⬆️
sklearn/datasets/samples_generator.py 93.42% <100%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9172a59...8a86067. Read the comment docs.

Christian Braune
@qinhanmin2014

LGTM except for something to confirm from core devs. Also wondering if it will be helpful to explicitly state that the center of the two circles is fixed to (0,0) and the radius of the outer circle is 1.

@@ -611,22 +612,25 @@ def make_circles(n_samples=100, shuffle=True, noise=None, random_state=None,
The integer labels (0 or 1) for class membership of each sample.
"""
if factor > 1 or factor < 0:
if factor >= 1 or factor < 0:

This comment has been minimized.

@qinhanmin2014

qinhanmin2014 Nov 3, 2017

Member

Personally, I'm with you. But this latest source code change is not reviewed by anyone else. So need to confirm with core devs to see whether we need factor=1 in extreme case.

@qinhanmin2014

qinhanmin2014 Nov 3, 2017

Member

Personally, I'm with you. But this latest source code change is not reviewed by anyone else. So need to confirm with core devs to see whether we need factor=1 in extreme case.

This comment has been minimized.

@christianbraune79

christianbraune79 Nov 3, 2017

Contributor

Yes, definitely. If it was a typo in the docs then I'll revert the change and adjust the documentation. :)

@christianbraune79

christianbraune79 Nov 3, 2017

Contributor

Yes, definitely. If it was a typo in the docs then I'll revert the change and adjust the documentation. :)

This comment has been minimized.

@jnothman

jnothman Nov 4, 2017

Member

I this is fine.

@jnothman

jnothman Nov 4, 2017

Member

I this is fine.

@jnothman

Otherwise LGTM

Show outdated Hide outdated doc/whats_new/_contributors.rst Outdated
@@ -611,22 +612,25 @@ def make_circles(n_samples=100, shuffle=True, noise=None, random_state=None,
The integer labels (0 or 1) for class membership of each sample.
"""
if factor > 1 or factor < 0:
if factor >= 1 or factor < 0:

This comment has been minimized.

@jnothman

jnothman Nov 4, 2017

Member

I this is fine.

@jnothman

jnothman Nov 4, 2017

Member

I this is fine.

Show outdated Hide outdated sklearn/datasets/tests/test_samples_generator.py Outdated

@jnothman jnothman changed the title from [MRG + 1] make_circles() now works with odd number of samples, test added to [MRG+2] make_circles() now works with odd number of samples, test added Nov 4, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Nov 8, 2017

Member

@christianbraune79 Could you please take some time to finish this, it's very close form merge.
(1) please remove your name in _contributors.rst, that's for core devs
(2) please extend the test to even case, you might consider a loop to avoid duplicate code. Also consider to add a comment to explain that we are testing both odd case and even case here.

Member

qinhanmin2014 commented Nov 8, 2017

@christianbraune79 Could you please take some time to finish this, it's very close form merge.
(1) please remove your name in _contributors.rst, that's for core devs
(2) please extend the test to even case, you might consider a loop to avoid duplicate code. Also consider to add a comment to explain that we are testing both odd case and even case here.

Christian Braune added some commits Nov 9, 2017

@christianbraune79

This comment has been minimized.

Show comment
Hide comment
@christianbraune79

christianbraune79 Nov 9, 2017

Contributor

@qinhanmin2014
Done. Sorry for (1) taking so long to respond (family stuff) and (2) putting myself in the contributors list. At least now I learned, what it is for. :)

Contributor

christianbraune79 commented Nov 9, 2017

@qinhanmin2014
Done. Sorry for (1) taking so long to respond (family stuff) and (2) putting myself in the contributors list. At least now I learned, what it is for. :)

Christian Braune
@glemaitre

3 nitpicks more :)

Show outdated Hide outdated sklearn/datasets/tests/test_samples_generator.py Outdated
Show outdated Hide outdated sklearn/datasets/tests/test_samples_generator.py Outdated

Christian Braune added some commits Nov 10, 2017

Christian Braune
Christian Braune
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Nov 11, 2017

Member

@jnothman @amueller Could you please give a final check? I think it is OK for merge. Thanks :)

Member

qinhanmin2014 commented Nov 11, 2017

@jnothman @amueller Could you please give a final check? I think it is OK for merge. Thanks :)

@jnothman jnothman merged commit 4bead39 into scikit-learn:master Nov 11, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.19%)
Details
codecov/project 96.2% (+0.01%) compared to 9172a59
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Nov 11, 2017

Member

@christianbraune79 Thanks for the issue and the PR :)

Member

qinhanmin2014 commented Nov 11, 2017

@christianbraune79 Thanks for the issue and the PR :)

@christianbraune79 christianbraune79 deleted the christianbraune79:make_circles branch Nov 11, 2017

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