Skip to content

add RandomVerticalFlip transform #262

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

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

alykhantejani
Copy link
Contributor

@alykhantejani alykhantejani commented Sep 17, 2017

Closes #257.

I've currently done the same as RandomHorizontalFlip which uses a fixed probability of flipping of 50% and I've only used this probability in the tests.

This is easy to extend to have the probability of flipping be different - but not sure if this is a valuable feature?

Also note, the tests for these require scipy which, at current, is an optional dependency for torchvision. @soumith Do we have scipy on the build box/ is there a way to have it installed for test runs?

Copy link
Member

@fmassa fmassa 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 this looks good!
We have already added support for scipy in the test machines, so I think this is fine.
About adding support for different flip probabilities, I think this can be added in a later PR if needed.

@alykhantejani
Copy link
Contributor Author

@fmassa shall we merge this now, or after #240 ?

@alykhantejani
Copy link
Contributor Author

@fmassa Having a look at the travis job config + log it looks like scipy isn't installed but should be easy to add. The test output also confirms that the test was skipped.

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.

[Feature Request] Add RandomVerticalFlip transform
2 participants