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

preprocess_img in tf #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

arnaudbenard
Copy link

@arnaudbenard arnaudbenard commented Aug 22, 2017

I have moved preprocess_img to preprocess_img_tf which does the preprocessing in TF. If you are happy with that we can remove the old preprocess_img method.

Let me know what you think!

cc @gyglim

@scaelles
Copy link
Owner

Thanks a lot for your contribution. However, if I understand correctly your code, there is one case that is not covered. In my Dataset class, there are two options whether to load everything to memory or just load it when you need it and then it provides a path instead of a numpy array. In the first case, your code would work. In the second case that is implemented using this:
if type(image) is not np.ndarray: image = np.array(Image.open(image), dtype=np.uint8)

in my original code it will not.

If you could incorporate this possibility in your pull request, it would be great!

@gyglim
Copy link

gyglim commented Aug 23, 2017

We'll do another pass over it. Anyway, the preprocessing is done inefficiently.

@arnaudbenard
Copy link
Author

@scaelles Thank you for the feedback, I will take the image loading use-case into account. I think @gyglim is right and we should move the preprocessing to the network to avoid creating a new graph for each execution. We will push an update!

Are there other functions you would like to move to TF?

@scaelles
Copy link
Owner

@gyglim I agree that the preprocessing is inefficient and should be move to a more TF friendly manner, but I haven't had the time to do that.
@arnaudbenard I think that the other function that can easily be moved to TF is preprocess_labels (I implemented it, but I didn't fully test it).

@bhack
Copy link

bhack commented Oct 28, 2017

I think also that the new TF dataset api could help to have a more clean solution

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