Fixed the confusion between the X and Y axes.

keras-team / keras-preprocessing

Utilities for working with image data, text data, and sequence data.

Other

1.02k stars 444 forks source link

Fixed the confusion between the X and Y axes. #318

Closed eli-osherovich closed 3 years ago

eli-osherovich commented 3 years ago

Summary

Related Issues

PR Overview

[y] This PR requires new unit tests [y/n] (make sure tests are included)
[y] This PR requires to update the documentation [y/n] (make sure the docs are up-to-date)
[n] This PR is backwards compatible [y/n]
[n] This PR changes the current API [y/n] (all API changes need to be approved by fchollet)

Fixes #315 Tests were update to circumvent #316

eli-osherovich commented 3 years ago

Sanity checks added in this PR revealed some buggy tests (incorrect axes' indices) Will be fixed in the next commit.

keras_preprocessing/image/affine_transformations.py:56: in random_rotation row_axis = 0, col_axis = 1, channel_axis = 0 keras_preprocessing/image/affine_transformations.py:153: in random_zoom row_axis = 0, col_axis = 1, channel_axis = 0

eli-osherovich commented 3 years ago

Second commit changes are partially related to #316

eli-osherovich commented 3 years ago

By the way, should new PRs support python2?

Dref360 commented 3 years ago

No it is not needed, but please edit the travis file accordingly. I cannot merge my own PRs unfortunatly so I can't do that myself.

eli-osherovich commented 3 years ago

Thanks for your prompt reply @Dref360, here it is: https://github.com/keras-team/keras-preprocessing/pull/319

Dref360 commented 3 years ago

I merge #319 so if you merge master with this branch we should LGTM

eli-osherovich commented 3 years ago

@Dref360 Done.

What is the policy for PR syncing: merge or rebase?

Dref360 commented 3 years ago

Merge is good!