Skip to content
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

Minor change: Added tif and tiff to white_list_formats #9187

Merged
merged 3 commits into from
Jan 27, 2018

Conversation

MiliasV
Copy link
Contributor

@MiliasV MiliasV commented Jan 25, 2018

Hello there,
It just took me some time to understand why "flow_from_directory" didn't work for me, only to realize that the "tif" format was not in the white_list_formats. So, I added it :) .

@Dref360
Copy link
Contributor

Dref360 commented Jan 25, 2018

This could be an issue for a lot of people. PIL doesn't work if the tiff has more than 3 channels.
python-pillow/Pillow#1888

@fchollet
Copy link
Member

@Dref360 your call.

@MiliasV
Copy link
Contributor Author

MiliasV commented Jan 26, 2018

I don't have a counter argument. If you think that it is going to cause more problems than it will solve I understand. Let me know 😄.

@Dref360
Copy link
Contributor

Dref360 commented Jan 26, 2018

@CharlesAuthier is working in my lab and has more experience on this type of file.

@Dref360
Copy link
Contributor

Dref360 commented Jan 26, 2018

The only workaround I see is to use Pillow and libtiff instead of PIL. This is a major change so we shouldn't go this way.

According to Charles, if you have RGB-IR data (or any multiband data), the output will be deformed by PIL.

It won't affect most people. So I would like to merge it, but maybe add a warning in _count_valid_files_in_directory if there is a tiff. Something along the line of :

UserWarning('Using \'.tiff\' files with multiple bands will cause distortion.',
                      'Please verify your output.')

@MiliasV
Copy link
Contributor Author

MiliasV commented Jan 26, 2018

Something like that 🤔 ?

@@ -963,6 +963,9 @@ def _recursive_list(subpath):
for fname in files:
is_valid = False
for extension in white_list_formats:
if fname.lower().endswith('.tiff'):
Copy link
Contributor

Choose a reason for hiding this comment

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

This will warns a tremendous amount times, isn't it?
I would just do a check before returning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am pretty sure it won't (I also ran it).

See the docs at http://docs.python.org/2/library/warnings.html:

"Repetitions of a particular warning for the same source location are typically suppressed."

Copy link
Contributor

Choose a reason for hiding this comment

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

Good then!

@Dref360
Copy link
Contributor

Dref360 commented Jan 26, 2018

LGTM for me. I don't think we need to test that the warning is raised? (It's sometime a pain with pytest)

@MiliasV
Copy link
Contributor Author

MiliasV commented Jan 26, 2018

If you ask me, I reckon it's ok :)

@@ -963,6 +963,9 @@ def _recursive_list(subpath):
for fname in files:
is_valid = False
for extension in white_list_formats:
if fname.lower().endswith('.tiff'):
warnings.warn('Using \'.tiff\' files with multiple bands will cause distortion.'
'Please verify your output.')
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO yes or we shouldn't accept tiff at all.
We'll get complaints that some .tiff are not handled correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Ok then. The message is missing a space for the period, please fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. Fixed.

@fchollet fchollet merged commit 83ae7d6 into keras-team:master Jan 27, 2018
@fanshius
Copy link

you can temporarily solve this problem by adding 'tif', 'tiff' in the format list in image.py source code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants