Removed SciPy dependency.

keras-team / keras-preprocessing

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

Other

1.02k stars 444 forks source link

Removed SciPy dependency. #334

Open eli-osherovich opened 3 years ago

eli-osherovich commented 3 years ago

Summary

Related Issues

PR Overview

[n] This PR requires new unit tests [y/n] (make sure tests are included)
[n] This PR requires to update the documentation [y/n] (make sure the docs are up-to-date)
[y] 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)

This is the second (last) part of the effort to remove SciPy dependency (#322)

Note: currently two tests are failing because of a bug tensorflow/addons#2357. Since the issue is not critical -- it only affects boundaries, we can adjust unit tests to follow the buggy behavior of tensorflow-addons.

Dref360 commented 3 years ago

Switching to tf_addons is a pretty big change so I will reach out to Francois.

Thank you for the PR!

eli-osherovich commented 3 years ago

I think, there's no reason to keep "external" dependencies. I would suggest to get rid of numpy too.

On Thu, Jan 21, 2021, 6:19 PM Frédéric Branchaud-Charron < notifications@github.com> wrote:

Switching to tf_addons is a pretty big change so I will reach out to Francois.

Thank you for the PR!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/keras-team/keras-preprocessing/pull/334#issuecomment-764762818, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASS73RSVVMIYW4Z7YQCZULS3BHZDANCNFSM4WMUHG6A .

eli-osherovich commented 3 years ago

In fact we do not need anything but TF here. Instead of tensorflow_addons one can use here tf.raw_ops.ImageProjectiveTransformV3

P. S. updated the PR to use tensorflow only.

Dref360 commented 3 years ago

Once tensorflow/tensorflow#46637 is merged I think we can merge this pr.

Thank you!

eli-osherovich commented 3 years ago

Once tensorflow/tensorflow#46637 is merged I think we can merge this pr.

Thank you!

They will not approve it because it breaks internal tests (that are relying on the current behavior). Should we update our unit tests to rely on the TF behavior?