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] Fix for image.extract_patches_2d when # of patches requested > all possible patches #10101

Merged
merged 3 commits into from Nov 25, 2017

Conversation

Projects
None yet
3 participants
@varunagrawal

varunagrawal commented Nov 9, 2017

Reference Issues/PRs

Fixes #10100

What does this implement/fix? Explain your changes.

This updates the _compute_n_patches function to check if the max_patches value >= all_patches and return the appropriate value for number of patches.

Any other comments?

@varunagrawal varunagrawal changed the title from Fix for `image.extract_patches_2d` when # of patches requested > all possible patches to Fix for image.extract_patches_2d when # of patches requested > all possible patches Nov 9, 2017

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Nov 9, 2017

Member

please add a test

Member

agramfort commented Nov 9, 2017

please add a test

@varunagrawal

This comment has been minimized.

Show comment
Hide comment
@varunagrawal

varunagrawal Nov 9, 2017

Added 2 tests

varunagrawal commented Nov 9, 2017

Added 2 tests

@glemaitre

Couple of nitpicks. Otherwise LGTM

Show outdated Hide outdated sklearn/feature_extraction/tests/test_image.py Outdated
Show outdated Hide outdated sklearn/feature_extraction/tests/test_image.py Outdated
Show outdated Hide outdated sklearn/feature_extraction/tests/test_image.py Outdated

@varunagrawal varunagrawal changed the title from Fix for image.extract_patches_2d when # of patches requested > all possible patches to [MRG] Fix for image.extract_patches_2d when # of patches requested > all possible patches Nov 10, 2017

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Nov 10, 2017

Member

just don't forget to update what's new

Member

agramfort commented Nov 10, 2017

just don't forget to update what's new

@varunagrawal

This comment has been minimized.

Show comment
Hide comment
@varunagrawal

varunagrawal Nov 14, 2017

@agramfort can you please elaborate some more on your last comment? I seem to have lost the context in which it was made.

varunagrawal commented Nov 14, 2017

@agramfort can you please elaborate some more on your last comment? I seem to have lost the context in which it was made.

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Nov 14, 2017

Member

You need to document the change in the whats_new.rst page

Member

agramfort commented Nov 14, 2017

You need to document the change in the whats_new.rst page

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 15, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10101      +/-   ##
==========================================
+ Coverage    96.1%    96.1%   +<.01%     
==========================================
  Files         337      337              
  Lines       63295    63309      +14     
==========================================
+ Hits        60828    60842      +14     
  Misses       2467     2467
Impacted Files Coverage Δ
sklearn/feature_extraction/image.py 100% <100%> (ø) ⬆️
sklearn/feature_extraction/tests/test_image.py 100% <100%> (ø) ⬆️
sklearn/linear_model/sag.py 94.36% <0%> (ø) ⬆️

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 9808810...f19b1e1. Read the comment docs.

codecov bot commented Nov 15, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10101      +/-   ##
==========================================
+ Coverage    96.1%    96.1%   +<.01%     
==========================================
  Files         337      337              
  Lines       63295    63309      +14     
==========================================
+ Hits        60828    60842      +14     
  Misses       2467     2467
Impacted Files Coverage Δ
sklearn/feature_extraction/image.py 100% <100%> (ø) ⬆️
sklearn/feature_extraction/tests/test_image.py 100% <100%> (ø) ⬆️
sklearn/linear_model/sag.py 94.36% <0%> (ø) ⬆️

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 9808810...f19b1e1. Read the comment docs.

@varunagrawal

This comment has been minimized.

Show comment
Hide comment
@varunagrawal

varunagrawal Nov 15, 2017

@agramfort thank you for the info. I have updated the document.

varunagrawal commented Nov 15, 2017

@agramfort thank you for the info. I have updated the document.

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

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 22, 2017

@varunagrawal could you solve the conflict and address @agramfort comments

glemaitre commented Nov 22, 2017

@varunagrawal could you solve the conflict and address @agramfort comments

@varunagrawal

This comment has been minimized.

Show comment
Hide comment
@varunagrawal

varunagrawal Nov 23, 2017

@glemaitre I have made the requested changes. Apologies for the delay.

varunagrawal commented Nov 23, 2017

@glemaitre I have made the requested changes. Apologies for the delay.

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Nov 23, 2017

Member

don't document changes in private function. Say what public function is impacted.

btw see how doc renders: https://15080-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/whats_new.html

Member

agramfort commented Nov 23, 2017

don't document changes in private function. Say what public function is impacted.

btw see how doc renders: https://15080-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/whats_new.html

@varunagrawal

This comment has been minimized.

Show comment
Hide comment
@varunagrawal

varunagrawal Nov 24, 2017

@agramfort sorry about that. Still getting used to the way things are done here.
Hopefully the current changes are to your satisfaction. Thank you again for your immense patience.

varunagrawal commented Nov 24, 2017

@agramfort sorry about that. Still getting used to the way things are done here.
Hopefully the current changes are to your satisfaction. Thank you again for your immense patience.

@glemaitre glemaitre changed the title from [MRG] Fix for image.extract_patches_2d when # of patches requested > all possible patches to [MRG + 1] Fix for image.extract_patches_2d when # of patches requested > all possible patches Nov 25, 2017

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 25, 2017

Merging since only what's new was pending.

Thanks for your contribution.

glemaitre commented Nov 25, 2017

Merging since only what's new was pending.

Thanks for your contribution.

@glemaitre glemaitre merged commit 6eb218e into scikit-learn:master Nov 25, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.1%)
Details
codecov/project 96.1% (+<.01%) compared to 9808810
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

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

@varunagrawal varunagrawal deleted the varunagrawal:sample-patches-fix branch Feb 8, 2018

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