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

[WIP] Add Numpy Backend #87

Closed
wants to merge 54 commits into from
Closed

[WIP] Add Numpy Backend #87

wants to merge 54 commits into from

Conversation

ariG23498
Copy link
Collaborator

@ariG23498 ariG23498 commented May 4, 2023

This PR will add NumPy backend to KerasCore

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/__init__.py Outdated Show resolved Hide resolved
@ariG23498 ariG23498 requested a review from fchollet May 4, 2023 18:50
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR -- it looks good overall, what test failures do you see?

keras_core/backend/numpy/core.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/core.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/trainer.py Outdated Show resolved Hide resolved
keras_core/backend/numpy/layer.py Show resolved Hide resolved
@ariG23498
Copy link
Collaborator Author

Every test realted to the Trainer fails (as NumPy should not have a trainer module). It would be great if someone from the team could help me understand whether the failing tests should be skipped or I should rather work on them to make them pass.

@AakashKumarNain
Copy link
Collaborator

I can take a look, and will try to help

@fchollet
Copy link
Member

fchollet commented Jul 6, 2023

Every test realted to the Trainer fails (as NumPy should not have a trainer module). It would be great if someone from the team could help me understand whether the failing tests should be skipped or I should rather work on them to make them pass.

Normally every test that is "supposed" to fail should fail with a NotImplementedError exception. That's the way to tell.

@ariG23498
Copy link
Collaborator Author

ariG23498 commented Jul 7, 2023

Todos:

  • Testing on metrics -- Confusion Metric to be specific
  • Testing on layers
  • Testing on ops -- All the image related utilities

@ariG23498
Copy link
Collaborator Author

@AakashKumarNain if you want to take up the image.py file that would be of immense help! I am having a hard time to aligning the resize methods with PIL.

@ariG23498
Copy link
Collaborator Author

@fchollet would it be okay if I introduced the numpy test in the GitHub Actions as well?

And to do that I think I would just need to add numpy here

backend: [tensorflow, jax, torch]

Am I correct here?

@ariG23498
Copy link
Collaborator Author

@fchollet some pointers:

  • In the current state only tests for rnn, lstm, and gru fail. How would you like me to proceed with this?
  • For the image.py I have use JAX (also suggested by @AakashKumarNain) against PIL. To begin with PIL cannot handle float32 with RGB images, conversions are lossy and inherently tests always fail. Using JAX also makes sense here as the conv operation in numpy already uses JAX (making it a dependency). [WIP] Add Numpy Backend #87 (comment)
  • [WIP] Add Numpy Backend #87 (comment)

@AakashKumarNain
Copy link
Collaborator

It is a known issue that resizing with PIL though most correct, is impossible to reproduce in opencv or any other library per se. Every library has very minor difference for different interpolations.

@fchollet
Copy link
Member

Let's carry on in the other PR!

@fchollet fchollet closed this Jul 16, 2023
@fchollet fchollet deleted the aritra-np-backend branch July 17, 2023 18:32
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.

3 participants