search

keras-team / keras-preprocessing

Utilities for working with image data, text data, and sequence data.
Other
1.02k stars 444 forks source link

Get rid of scipy #322

Open eli-osherovich opened 3 years ago

eli-osherovich commented 3 years ago

Please make sure that the boxes below are checked before you submit your issue. If your issue is an implementation question, please ask your question on StackOverflow or on the Keras Slack channel instead of opening a GitHub issue.

Thank you!

From my experience, relying on SciPy for image transformation makes it: a) slow b) non-suitable for TPU

Is there any particular reason to keep SciPy in the loop?

Stanfording commented 3 years ago

For the m1 apple chip, SciPy is not available yet, but the ImageDataGenerator.flow_from_directory requires it. So getting rid of scipy would be a temporary relief for the m1 users. Maybe not necessary though ( since SciPy is working on it too I guess).

eli-osherovich commented 3 years ago

I can do this if there is a real need from the keras side.

Dref360 commented 3 years ago

We use scipy here for SVD (I think np.linalg.svd is called?) here

And we use scipy.ndimage for matrix multiplication here: ndimage.interpolation.affine_transform.

Do you think it would require a lot of work to replicate affine_transform behaviour?

eli-osherovich commented 3 years ago

No. It should be pretty simple.

On Thu, Jan 14, 2021, 4:30 PM Frédéric Branchaud-Charron < notifications@github.com> wrote:

We use scipy here for SVD (I think np.linalg.svd is called?) here https://github.com/keras-team/keras-preprocessing/blob/54b5d6ed65f1608ca7207d585075c2ad3a52929e/keras_preprocessing/image/image_data_generator.py#L986

And we use scipy.ndimage for matrix multiplication here: ndimage.interpolation.affine_transform https://github.com/keras-team/keras-preprocessing/blob/54b5d6ed65f1608ca7207d585075c2ad3a52929e/keras_preprocessing/image/affine_transformations.py#L387 .

Do you think it would require a lot of work to replicate affine_transform behaviour?

— 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/issues/322#issuecomment-760232103, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASS73X5YPPFSVXBJFJPWATSZ35YPANCNFSM4TGMG7JQ .

Dref360 commented 3 years ago

PRs welcome then :)

eli-osherovich commented 3 years ago

Will send some shortly

On Fri, Jan 15, 2021, 6:16 PM Frédéric Branchaud-Charron < notifications@github.com> wrote:

PRs welcome then :)

— 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/issues/322#issuecomment-761036321, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASS73UOJAEUXWZTKVEXEHLS2BS5NANCNFSM4TGMG7JQ .

Stanfording commented 3 years ago

I can do this if there is a real need from the keras side.

Yes, please! Because scipy is important to Keras and the machine learning community.

eli-osherovich commented 3 years ago

Yes, please! Because scipy is important to Keras and the machine learning community.

Just to make sure: the idea is to remove scipy dependency from keras. Namely, it will not make scipy working on M1, we'll just replace it with something else so as to let keras be used on M1.

Stanfording commented 3 years ago

Yes, please! Because scipy is important to Keras and the machine learning community.

Just to make sure: the idea is to remove scipy dependency from keras. Namely, it will not make scipy working on M1, we'll just replace it with something else so as to let keras be used on M1.

That's exactly what I meant!

eli-osherovich commented 3 years ago

@Stanfording first patch is waiting for approval, as you can see. I chose to replace scipy with numpy, but I cannot test it on M1.... Actually, after thinking about it a bit, I do not know if numpy is working on M1...

Stanfording commented 3 years ago

@Stanfording first patch is waiting for approval, as you can see. I chose to replace scipy with numpy, but I cannot test it on M1.... Actually, after thinking about it a bit, I do not know if numpy is working on M1...

There are some issues with numpy if just downloading by itself. However, numpy works with tensorflow if downloading tensorflow with this link https://github.com/apple/tensorflow_macos . So many people who cannot download numpy on m1 just download the TensorFlow from the link above, which will install numpy automatically. The version of this numpy is currently 1.18.5. Python version is 3.8.2 from xcode command line tool. The way to use keras on m1 is mostly use "from tensorflow import keras" instead of downloading keras directly and just import keras. So if there is any update from keras, we can only use it from the one imported from tensorflow. I hope the information helps.

Stanfording commented 3 years ago

@Stanfording first patch is waiting for approval, as you can see. I chose to replace scipy with numpy, but I cannot test it on M1.... Actually, after thinking about it a bit, I do not know if numpy is working on M1...

However, it is possible for m1 users to download keras directly. But it's easy to run into many issues. I spent a lot of time to solve issues and finally get keras downloaded successfully in the same virtual environment with tensorflow. However, when using it, I could just type "python3" and "import keras" on the terminal in the virtual environment, which imported successfully, but if I import keras in a text editor (spyder) and use "python3 xxx.py" in virtual environment, it will say "No module named 'keras'". If this problem is impossible to solve by users, then the only way we can get keras is this link: https://github.com/apple/tensorflow_macos. I hope this is also helpful.