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

fb.resnet like transforms #27

Closed
wants to merge 1 commit into from
Closed

fb.resnet like transforms #27

wants to merge 1 commit into from

Conversation

eladhoffer
Copy link
Contributor

No description provided.

@fmassa
Copy link
Member

fmassa commented Jan 19, 2017

I was wondering if it would make sense to add a inplace option to the constructor, that modifies the input tensor inplace.
This would only be a tiny optimization to avoid a copy+clone, don't know if it's worth it though.

@Maratyszcza
Copy link
Contributor

@eladhoffer Very nice! Do you have an estimate of how these transforms improve accuracy for ImageNet models?

@eladhoffer
Copy link
Contributor Author

Actually, I didn't find any difference for most models...

@fmassa
Copy link
Member

fmassa commented Mar 13, 2017

@soumith @colesbury Shall we merge this (after a rebase/review)? These transforms don't seem to help improve performance on classification tasks, but users might want to try it out anyway for their own tasks.
cc @alykhantejani

@Maratyszcza
Copy link
Contributor

@fmassa I am struggling to train tiny darknet model to reported accuracy, and thus far I matched everything, but the extra pre-processing transformations. It seems likely that extra transformations matter and are be necessary to reproduce the result on this classifier.

@alykhantejani
Copy link
Contributor

@Maratyszcza did you find these transforms necessary to reproduce tiny darknet in the end?

@alykhantejani
Copy link
Contributor

Hi @eladhoffer sorry for the delayed response on this - are you still interested in adding these? If not, we can get round to adding them in the next few weeks.

I think Brightness, Staturation and Contrast are valuable and bring us up to speed with what tensorflow have (see #271 for the list).

@eladhoffer
Copy link
Contributor Author

Sure, how can I help?

@chsasank chsasank mentioned this pull request Sep 27, 2017
@alykhantejani
Copy link
Contributor

@eladhoffer Awesome. I think the following changes are needed:

  1. The transforms at present appear to be based on Tensor inputs, but we seemed to have converged to use PIL/accimage Image inputs everywhere
  2. We now have a functional interface and the objects are just a thin wrapper around functions, this will eventually allow us to do things like RandomOrder from this PR in one function as a transform (randomly calling other functions)
  3. it would be good if you could add some tests for these transforms.

It'd be great if you have the time to make these changes to this PR, if not @chsasank has started implementing some other color transforms #275 - in any case it's probably worth you guys coordinating as to not duplicate work.

@chsasank
Copy link
Contributor

Hey @eladhoffer, I am almost done implementing all the colour transforms with PIL in #275. So I'll just continue the PR and complete it. This is fine?

@eladhoffer
Copy link
Contributor Author

Sure, let me know if you need any help.

@alykhantejani
Copy link
Contributor

Closing this then as this will be implemented in #275

rajveerb pushed a commit to rajveerb/vision that referenced this pull request Nov 30, 2023
Encountered using flake8 testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants