DOC Adds an example to PatchExtractor #12819

CatChenal · 2018-12-18T16:55:18Z

What does this implement/fix?

Completes #12202 fix abandoned by @parul-l.

Modifications made as per @amueller and @adrinjalali:
Replaced random data with an image from sklearn image dataset;
Reformated the output.

Any other comments?

This completed fix results from Reshama Shaikh's push to close all PRs opened during WiMLDS's scikit-learn sprint in Sep. 2018.
Yes, Reshama is herding cats!

cc: @reshamas

jeremiedbb · 2018-12-18T17:02:30Z

Thanks.
Please give a more informative title to your PR
Also please add Fix #12202 to the description of you PR so that the issue will be closed when you PR is merged.

Finally, I think the example should be added to the class docstring, not the transform method docstring, no ?

reshamas · 2018-12-18T17:36:06Z

@CatChenal Thanks so much for assisting with this list of PRs!

Possible title for PR: "example added to the feature_extraction.image.PatchExtractor"
Here is a reference for Closing Issues Using Keywords: "This closes [MRG] Added an example to the sklearn.feature_extraction.image.PatchExtractor #12202 and references Add examples to class docs #3846"

CatChenal · 2018-12-18T17:46:52Z

Thanks.
Please give a more informative title to your PR
Also please add Fix #12202 to the description of you PR so that the issue will be closed when you PR is merged.

Finally, I think the example should be added to the class docstring, not the transform method docstring, no ?

I agree that the example should be at the PatchExtractor class level, but this issue was not addressed by the original PR's two commenters, @amueller or @adrinjalali, therefore I did not move the example.

Should I do so?

Also note that the example under the extract_patches_2d method (line 339) in feature_extaction/image submodule suffers from the same objection by @amueller in #12202, namely the use of synthetic array data while there are small built-in images at the ready.
Would that constitute a new issue, e.g. "Data source in examples for small image processing: use built-in images, not random arrays", or should I perform the move and the image source modification in the same pull?

glemaitre

I pointed only 2 issues but they are at multiple locations.

glemaitre · 2018-12-18T18:50:58Z

sklearn/feature_extraction/image.py

-    >>> patches[8]
-    array([[10, 11],
-           [14, 15]])
+        from sklearn.datasets import load_sample_images


You should use a 4 spaces indent. It looks like a tab instead. You should also prepend >>> before a command as in the previous example.

I ran Flake8 on sklearn/feature_extraction/image.py: there was no output, hence I took that to mean that there was no style issue.
I'll fix that momentarily.

glemaitre · 2018-12-18T18:56:46Z

sklearn/feature_extraction/image.py

+
+        # Use the array data from the first image in this dataset:
+        one_image = load_sample_images().images[0]
+        print(f'Image shape: {one_image.shape}')


This string formatting is only available in python 3.6. We are still supporting python 3.5. Use normal string with format here

reshamas · 2018-12-18T19:01:14Z

@CatChenal to update the description, at the very top, on the upper right are "..."
Click on those 3 dots and there will an option to edit the text of the description of the PR.

adrinjalali · 2018-12-18T20:28:28Z

The scope of the original issue was to add examples to the class docstrings, I guess we can keep it that way and avoid including examples to functions' docstrings, at least for now.

parul-l · 2018-12-19T01:39:26Z

Thank you all and sorry about neglecting it.

jnothman · 2018-12-19T14:05:09Z

sklearn/feature_extraction/image.py

+        >>> one_image = load_sample_images().images[0]
+
+        >>> print('Image shape: {}'.format(one_image.shape))
+


The output should be immediately after the statement that produced it

reshamas · 2019-01-05T00:55:04Z

@CatChenal
Happy New Year!
Will you be able to continue work on this PR?

CatChenal · 2019-01-05T15:36:37Z

Happy New Year to you too, @reshamas.
Thanks for the reminder.

Could please you confirm my understanding of the failed check?
As per the 'Resolve conflict' page, it is an indentation issue.
Thanks.

reshamas · 2019-01-05T15:43:15Z

@CatChenal
It looks like it. Reminder to update the description of the PR as well. Thanks.
Give it a try, and let's keep our fingers crossed that all checks pass.

adrinjalali · 2019-01-05T16:06:54Z

test failing, you also need to merge master into your branch to fix the conflicts.

… (#wimlds)

CatChenal · 2019-01-05T18:01:48Z

I don't know what to do now: "Already up to date." is the result of git merge master.

adrinjalali · 2019-01-05T19:33:17Z

You need to have and upstream set up in your local clone and merge from that. There's a bit of documentation with some good keywords here: https://scikit-learn.org/dev/developers/contributing.html#how-to-contribute

As for this PR, it's easier if you get the output from a script such as this one: https://gist.github.com/adrinjalali/eca43556ca1620c72d91d8326ca59587

You can also check out some of the related PRs linked in this issue: #3846

…g (#wimlds)

CatChenal · 2019-01-16T16:45:03Z

This worked:

    >>> print(patches[1]) # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE
    [[[174 201 231]
      [174 201 231]]
     [[173 200 230]
      [173 200 230]]]

Thank you @jnothman @adrinjalali @reshamas

…addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE +ELLIPSIS for print statements.

…addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE for print statements.

jnothman · 2019-01-16T23:52:31Z

sklearn/feature_extraction/image.py

+    >>>
+    >>> # Here are just two of these patches:
+    >>> print(patches[1]) # doctest: +NORMALIZE_WHITESPACE \
+                                     +DONT_ACCEPT_BLANKLINE


I don't think you should need DONT_ACCEPT_BLANKLINE. Are you sure it's necessary?

@CatChenal can you give it a try without the DONT_ACCEPT_BLANKLINE option?

@reshamas

…DONT_ACCEPT_BLANKLINE (@jnothmam, @reshamas)

CatChenal · 2019-01-25T21:27:52Z

At last: LGTM!

adrinjalali · 2019-01-27T12:40:15Z

sklearn/feature_extraction/image.py

-           [ 4,  5,  6,  7],
-           [ 8,  9, 10, 11],
-           [12, 13, 14, 15]])
+    >>>


please remove these empty lines

adrinjalali

otherwise LGTM! Thanks @CatChenal

reshamas · 2019-01-30T17:49:29Z

@CatChenal It looks like there is one last request from @adrinjalali to remove some empty lines. Hopefully once that is completed by you, the PR can be merged in.

CatChenal · 2019-01-30T17:54:15Z

@reshamas My last push took care of it.

reshamas · 2019-01-30T18:00:18Z

@CatChenal I still see the empty lines beginning with >>>
notice the example in here does not have any blank lines: https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/feature_extraction/image.py

CatChenal · 2019-01-30T18:29:20Z

@reshamas You were right! My bad!

…xample.

CatChenal · 2019-01-30T18:44:16Z

I don't understand the new failure on CI: relates to #13067??

adrinjalali · 2019-01-31T09:18:11Z

Thanks @CatChenal !

reshamas · 2019-01-31T13:43:35Z

Thanks @CatChenal 👏 - I've updated the Sprint Impact Report with your merge.

Thanks @adrinjalali and @jnothman.

CatChenal · 2019-01-31T14:42:56Z

Thank you all @adrinjalali, @jnothman, and @reshamas.
My future contributions will be better managed from the lessons learned here.

@parul-l

* Finalizes fix for scikit-learn#12202 from abandonned PR by @parul-l * Completes 12202 fix abandoned by @parul-l * Completes 12202 fix abandoned by @parul-l * Extends 12202 fix over feature_extraction/image.py * Closes scikit-learn#12202; white space removal * Closes scikit-learn#12202; white space removal2 * Closes scikit-learn#12202; 3.5 compliance; added >>> in docstring code. * Closes scikit-learn#12202; indentation discrep. * Closes scikit-learn#12202; indentation discrep.2 * Example output formating; @jnotham * Example output formating; forgot flake8 * Closes scikit-learn#12202; Removed excessive indentation in docstring (#wimlds) * Closes scikit-learn#12202; Fixed inconsistent indentation in docstring (#wimlds) * Closes scikit-learn#12202 (#wimlds); intentation, v3.5 compliance * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE +ELLIPSIS for print statements. * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE for print statements. * Closes scikit-learn#12202 (#wimlds); Testing doctest direc.: removed DONT_ACCEPT_BLANKLINE (@jnothmam, @reshamas) * Closes scikit-learn#12202 (#wimlds); Removed blank lines in doctest example.

@parul-l

* Finalizes fix for scikit-learn#12202 from abandonned PR by @parul-l * Completes 12202 fix abandoned by @parul-l * Completes 12202 fix abandoned by @parul-l * Extends 12202 fix over feature_extraction/image.py * Closes scikit-learn#12202; white space removal * Closes scikit-learn#12202; white space removal2 * Closes scikit-learn#12202; 3.5 compliance; added >>> in docstring code. * Closes scikit-learn#12202; indentation discrep. * Closes scikit-learn#12202; indentation discrep.2 * Example output formating; @jnotham * Example output formating; forgot flake8 * Closes scikit-learn#12202; Removed excessive indentation in docstring (#wimlds) * Closes scikit-learn#12202; Fixed inconsistent indentation in docstring (#wimlds) * Closes scikit-learn#12202 (#wimlds); intentation, v3.5 compliance * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE +ELLIPSIS for print statements. * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE for print statements. * Closes scikit-learn#12202 (#wimlds); Testing doctest direc.: removed DONT_ACCEPT_BLANKLINE (@jnothmam, @reshamas) * Closes scikit-learn#12202 (#wimlds); Removed blank lines in doctest example.

@parul-l

* Finalizes fix for scikit-learn#12202 from abandonned PR by @parul-l * Completes 12202 fix abandoned by @parul-l * Completes 12202 fix abandoned by @parul-l * Extends 12202 fix over feature_extraction/image.py * Closes scikit-learn#12202; white space removal * Closes scikit-learn#12202; white space removal2 * Closes scikit-learn#12202; 3.5 compliance; added >>> in docstring code. * Closes scikit-learn#12202; indentation discrep. * Closes scikit-learn#12202; indentation discrep.2 * Example output formating; @jnotham * Example output formating; forgot flake8 * Closes scikit-learn#12202; Removed excessive indentation in docstring (#wimlds) * Closes scikit-learn#12202; Fixed inconsistent indentation in docstring (#wimlds) * Closes scikit-learn#12202 (#wimlds); intentation, v3.5 compliance * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE +ELLIPSIS for print statements. * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE for print statements. * Closes scikit-learn#12202 (#wimlds); Testing doctest direc.: removed DONT_ACCEPT_BLANKLINE (@jnothmam, @reshamas) * Closes scikit-learn#12202 (#wimlds); Removed blank lines in doctest example.

This reverts commit 9bcf9c9.

@parul-l

* Finalizes fix for scikit-learn#12202 from abandonned PR by @parul-l * Completes 12202 fix abandoned by @parul-l * Completes 12202 fix abandoned by @parul-l * Extends 12202 fix over feature_extraction/image.py * Closes scikit-learn#12202; white space removal * Closes scikit-learn#12202; white space removal2 * Closes scikit-learn#12202; 3.5 compliance; added >>> in docstring code. * Closes scikit-learn#12202; indentation discrep. * Closes scikit-learn#12202; indentation discrep.2 * Example output formating; @jnotham * Example output formating; forgot flake8 * Closes scikit-learn#12202; Removed excessive indentation in docstring (#wimlds) * Closes scikit-learn#12202; Fixed inconsistent indentation in docstring (#wimlds) * Closes scikit-learn#12202 (#wimlds); intentation, v3.5 compliance * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE +ELLIPSIS for print statements. * Closes scikit-learn#12202 (#wimlds); Output format issue solved with addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE for print statements. * Closes scikit-learn#12202 (#wimlds); Testing doctest direc.: removed DONT_ACCEPT_BLANKLINE (@jnothmam, @reshamas) * Closes scikit-learn#12202 (#wimlds); Removed blank lines in doctest example.

CatChenal added 3 commits December 18, 2018 11:38

Finalizes fix for scikit-learn#12202 from abandonned PR by @parul-l

5cb4bda

Completes 12202 fix abandoned by @parul-l

a39baed

Completes 12202 fix abandoned by @parul-l

a5b73bb

CatChenal added 3 commits December 18, 2018 13:03

Extends 12202 fix over feature_extraction/image.py

e9f9594

Closes scikit-learn#12202; white space removal

35da14a

Closes scikit-learn#12202; white space removal2

0730c7a

glemaitre requested changes Dec 18, 2018

View reviewed changes

Closes scikit-learn#12202; 3.5 compliance; added >>> in docstring code.

f81beda

CatChenal changed the title ~~Fixes #12202~~ Closes #12202 Dec 18, 2018

adrinjalali changed the title ~~Closes #12202~~ Adds an example to PatchExtractor Dec 18, 2018

CatChenal added 2 commits December 18, 2018 15:45

Closes scikit-learn#12202; indentation discrep.

294ce61

Closes scikit-learn#12202; indentation discrep.2

4f12330

jnothman reviewed Dec 19, 2018

View reviewed changes

CatChenal added 2 commits December 21, 2018 13:24

Example output formating; @jnotham

2a8d782

Example output formating; forgot flake8

d14b3b6

Closes scikit-learn#12202; Removed excessive indentation in docstring…

3f8b0b1

… (#wimlds)

CatChenal added 2 commits January 5, 2019 15:18

conflict resolution?

5fc5496

Closes scikit-learn#12202; Fixed inconsistent indentation in docstrin…

236dafb

…g (#wimlds)

CatChenal added 2 commits January 16, 2019 11:50

Closes scikit-learn#12202 (#wimlds); Output format issue solved with …

f173bf6

…addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE +ELLIPSIS for print statements.

Closes scikit-learn#12202 (#wimlds); Output format issue solved with …

aeae3b0

…addition of # doctest: +NORMALIZE_WHITESPACE +DONT_ACCEPT_BLANKLINE for print statements.

jnothman reviewed Jan 16, 2019

View reviewed changes

Closes scikit-learn#12202 (#wimlds); Testing doctest direc.: removed …

e38bca9

…DONT_ACCEPT_BLANKLINE (@jnothmam, @reshamas)

jnothman approved these changes Jan 27, 2019

View reviewed changes

jnothman changed the title ~~Adds an example to PatchExtractor~~ DOC Adds an example to PatchExtractor Jan 27, 2019

adrinjalali reviewed Jan 27, 2019

View reviewed changes

adrinjalali approved these changes Jan 27, 2019

View reviewed changes

Closes scikit-learn#12202 (#wimlds); Removed blank lines in doctest e…

f9352ab

…xample.

adrinjalali merged commit 3df0c19 into scikit-learn:master Jan 31, 2019

adrinjalali mentioned this pull request Jan 31, 2019

[MRG] Added an example to the sklearn.feature_extraction.image.PatchExtractor #12202

Closed

CatChenal deleted the myfeature branch January 31, 2019 14:42

qinhanmin2014 mentioned this pull request Feb 4, 2019

CI install pillow in Travis cron job #13080

Merged

rth mentioned this pull request Feb 13, 2019

failure: [doctest] sklearn.feature_extraction.image.extract_patches_2d #13152

Closed

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "DOC Adds an example to PatchExtractor (scikit-learn#12819)"

af34960

This reverts commit 9bcf9c9.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "DOC Adds an example to PatchExtractor (scikit-learn#12819)"

1b1254f

This reverts commit 9bcf9c9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC Adds an example to PatchExtractor #12819

DOC Adds an example to PatchExtractor #12819

CatChenal commented Dec 18, 2018 •

edited

Loading

jeremiedbb commented Dec 18, 2018

reshamas commented Dec 18, 2018

CatChenal commented Dec 18, 2018

glemaitre left a comment

glemaitre Dec 18, 2018

CatChenal Dec 18, 2018

glemaitre Dec 18, 2018

CatChenal Dec 18, 2018

reshamas commented Dec 18, 2018

adrinjalali commented Dec 18, 2018

parul-l commented Dec 19, 2018

jnothman Dec 19, 2018

reshamas commented Jan 5, 2019

CatChenal commented Jan 5, 2019

reshamas commented Jan 5, 2019

adrinjalali commented Jan 5, 2019

CatChenal commented Jan 5, 2019

adrinjalali commented Jan 5, 2019

CatChenal commented Jan 16, 2019

jnothman Jan 16, 2019

reshamas Jan 25, 2019

CatChenal commented Jan 25, 2019

adrinjalali Jan 27, 2019

adrinjalali left a comment

reshamas commented Jan 30, 2019

CatChenal commented Jan 30, 2019

reshamas commented Jan 30, 2019

CatChenal commented Jan 30, 2019

CatChenal commented Jan 30, 2019

adrinjalali commented Jan 31, 2019

reshamas commented Jan 31, 2019

CatChenal commented Jan 31, 2019

		>>> one_image = load_sample_images().images[0]

		>>> print('Image shape: {}'.format(one_image.shape))

DOC Adds an example to PatchExtractor #12819

DOC Adds an example to PatchExtractor #12819

Conversation

CatChenal commented Dec 18, 2018 • edited Loading

What does this implement/fix?

Any other comments?

jeremiedbb commented Dec 18, 2018

reshamas commented Dec 18, 2018

CatChenal commented Dec 18, 2018

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reshamas commented Dec 18, 2018

adrinjalali commented Dec 18, 2018

parul-l commented Dec 19, 2018

Choose a reason for hiding this comment

reshamas commented Jan 5, 2019

CatChenal commented Jan 5, 2019

reshamas commented Jan 5, 2019

adrinjalali commented Jan 5, 2019

CatChenal commented Jan 5, 2019

adrinjalali commented Jan 5, 2019

CatChenal commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CatChenal commented Jan 25, 2019

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

reshamas commented Jan 30, 2019

CatChenal commented Jan 30, 2019

reshamas commented Jan 30, 2019

CatChenal commented Jan 30, 2019

CatChenal commented Jan 30, 2019

adrinjalali commented Jan 31, 2019

reshamas commented Jan 31, 2019

CatChenal commented Jan 31, 2019

CatChenal commented Dec 18, 2018 •

edited

Loading