Skip to content

[WIP] Add Utility function to read image from the file as imread is deprecated#10149

Closed
keyur9 wants to merge 2 commits intoscikit-learn:masterfrom
keyur9:load_sample_images
Closed

[WIP] Add Utility function to read image from the file as imread is deprecated#10149
keyur9 wants to merge 2 commits intoscikit-learn:masterfrom
keyur9:load_sample_images

Conversation

@keyur9
Copy link
Copy Markdown

@keyur9 keyur9 commented Nov 16, 2017 •
edited
Loading

Reference Issues/PRs

Fixes #10147

What does this implement/fix? Explain your changes.

Copy the implementation of scipy.misc.imread as imread is deprecated suggested using PIL/pillow directly

Any other comments?

@keyur9 keyur9 changed the title Fixed Issue 10147: load_sample_images uses deprecated imread Fix Issue #10147: load_sample_images uses deprecated imread Nov 16, 2017
jnothman
jnothman reviewed Nov 16, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I think we should make _imread a function in sklearn.utils

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Nov 16, 2017 •
edited
Loading

I agree with @jnothman and you can add a test by using one of the image from sklearn.datasets.images

@glemaitre glemaitre changed the title Fix Issue #10147: load_sample_images uses deprecated imread [WIP] load_sample_images uses deprecated imread Nov 16, 2017
@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Nov 16, 2017

I put removed the issue number (it will happen automatically when merging). Could you give a better title to the PR

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 18, 2017

Codecov Report

Merging #10149 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10149   +/-   ##
=======================================
  Coverage   96.21%   96.21%           
=======================================
  Files         337      337           
  Lines       62891    62891           
=======================================
  Hits        60508    60508           
  Misses       2383     2383
Impacted Files Coverage Δ
sklearn/datasets/lfw.py 12.41% <0%> (ø) ⬆️
sklearn/datasets/tests/test_base.py 97.12% <100%> (ø) ⬆️
sklearn/datasets/base.py 85.5% <66.66%> (ø) ⬆️

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 f6163e7...6e952f1. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 18, 2017

Codecov Report

Merging #10149 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10149   +/-   ##
=======================================
  Coverage   96.21%   96.21%           
=======================================
  Files         337      337           
  Lines       62891    62891           
=======================================
  Hits        60508    60508           
  Misses       2383     2383
Impacted Files Coverage Δ
sklearn/datasets/lfw.py 12.41% <0%> (ø) ⬆️
sklearn/datasets/tests/test_base.py 97.12% <100%> (ø) ⬆️
sklearn/datasets/base.py 85.5% <66.66%> (ø) ⬆️

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 f6163e7...6e952f1. Read the comment docs.

@keyur9 keyur9 changed the title [WIP] load_sample_images uses deprecated imread [WIP] Add Utility function to read image from the file as imread is deprecated Nov 18, 2017
@keyur9
Copy link
Copy Markdown
Author

keyur9 commented Nov 18, 2017

Thank you for the feedback @jnothman and @glemaitre. I've created a utility function and added a test.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Nov 20, 2017

You have test failures...

jnothman
jnothman reviewed Nov 20, 2017
View reviewed changes
sklearn/utils/image_read.py
converted according to `mode`, and the result is then flattened using
mode 'F'.
"""
im = Image.open(name)
Copy link
Copy Markdown
Member

@jnothman jnothman Nov 20, 2017

Choose a reason for hiding this comment

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

You don't do anything here with flatten or mode.

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Dec 11, 2017

@keyur9 are you going to work on the fix?

@keyur9
Copy link
Copy Markdown
Author

keyur9 commented Dec 12, 2017

Hello @glemaitre , Apologize for losing track of this and not updating the PR. Sorry, to inform that I won't be able to work on this until the weekend. So, if anyone want to work on this, feel free to do that or else will get back to this on the weekend. Thank you,

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Dec 12, 2017 via email

@jotasi jotasi mentioned this pull request Jan 8, 2018
Merged
@qinhanmin2014
Copy link
Copy Markdown
Member

qinhanmin2014 commented Jan 18, 2018

Resolved in #10427 @keyur9 Thanks for your contribution.

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

Reviewers

@jnothman jnothman jnothman left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

load_sample_images uses deprecated imread

4 participants

@keyur9 @glemaitre @jnothman @qinhanmin2014