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

Add sparse input discretization test #18882

Conversation

AlvaroMaza
Copy link
Contributor

For my first commit to Keras, I picked the TODO in discretization_test.py.

Please let me know if I misunderstood the requirements, or otherwise made any mistakes.

Thank you!

Copy link

google-cla bot commented Dec 4, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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.

LGTM, thanks

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Dec 4, 2023
@fchollet
Copy link
Member

fchollet commented Dec 4, 2023

Please take a look at the test failures.

@google-ml-butler google-ml-butler bot removed the ready to pull Ready to be merged into the codebase label Dec 4, 2023
@AlvaroMaza
Copy link
Contributor Author

Changed it so that it checks if the backend is tensorflow

@@ -124,5 +125,11 @@ def test_saving(self):
self.assertAllClose(layer(ref_input), ref_output)

def test_sparse_inputs(self):
# TODO
pass
if backend.backend() != "tensorflow":
Copy link
Member

Choose a reason for hiding this comment

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

You should use a pytest decorator for this (we do this in a number of places elsewhere, check it out)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help, changed it to the decorator.

One small comment, when running

sh shell/format.sh

I had to change line isort --sp "${base_dir}/pyproject.toml" to isort -sp "${base_dir}/pyproject.toml" because I was getting this error:

usage: isort [-h] [-a ADD_IMPORTS] [-ac] [-af] [-b KNOWN_STANDARD_LIBRARY]
[-c] [-ca] [-cs] [-d] [-df] [-ds] [-dt] [-e]
[-f KNOWN_FUTURE_LIBRARY] [-fas] [-fass] [-ff FROM_FIRST]
[-fgw [FORCE_GRID_WRAP]] [-fss] [-i INDENT] [-j JOBS] [-k]
[-l LINE_LENGTH] [-lai LINES_AFTER_IMPORTS]
[-lbt LINES_BETWEEN_TYPES] [-le LINE_ENDING] [-ls]
[-m {0,1,2,3,4,5,6,7}] [-nis] [-nlb NO_LINES_BEFORE]
[-ns NOT_SKIP] [-o KNOWN_THIRD_PARTY] [-ot]
[-p KNOWN_FIRST_PARTY] [-q] [-r] [-rm REMOVE_IMPORTS] [-rr] [-rc]
[-s SKIP] [-sd DEFAULT_SECTION] [-sg SKIP_GLOB] [-sl]
[-sp SETTINGS_PATH] [-t FORCE_TO_TOP] [-tc] [-up] [-v] [-vb]
[--virtual-env VIRTUAL_ENV] [--conda-env CONDA_ENV] [-vn]
[-w LINE_LENGTH] [-wl WRAP_LENGTH] [-ws] [-y] [--unsafe]
[--case-sensitive] [--filter-files]
[files [files ...]]
isort: error: unrecognized arguments: --sp .

I am running the script using git bash

Copy link
Member

Choose a reason for hiding this comment

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

I had to change line isort --sp "${base_dir}/pyproject.toml" to isort -sp "${base_dir}/pyproject.toml" because I was getting this error:

It sounds like you have the wrong isort version then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was indeed running an old version. I updated it and run it, the imports should now be correctly sorted :)

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9620d23) 79.30% compared to head (b1ac669) 79.49%.
Report is 56 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #18882      +/-   ##
==========================================
+ Coverage   79.30%   79.49%   +0.18%     
==========================================
  Files         336      336              
  Lines       34549    34863     +314     
  Branches     6799     6853      +54     
==========================================
+ Hits        27400    27713     +313     
- Misses       5567     5572       +5     
+ Partials     1582     1578       -4     
Flag Coverage Δ
keras 79.34% <ø> (+0.17%) ⬆️
keras-jax 61.22% <ø> (-0.13%) ⬇️
keras-numpy 55.96% <ø> (-0.13%) ⬇️
keras-tensorflow 63.22% <ø> (-0.12%) ⬇️
keras-torch 63.82% <ø> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

LGTM, thank you

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Dec 11, 2023
Copy link

This PR is stale because it has been open for 14 days with no activity. It will be closed if no further activity occurs. Thank you.

@github-actions github-actions bot added the stale label Dec 26, 2023
@gbaned gbaned added ready to pull Ready to be merged into the codebase and removed stat:awaiting response from contributor stale ready to pull Ready to be merged into the codebase labels Dec 29, 2023
@davidmerwin
Copy link

@google-cla @github-actions @codecov-commenter #19024 #19023

Sent with GitHawk

@davidmerwin
Copy link

@google-cla @github-actions @codecov-commenter #19024 #19023 ...

@davidmerwin 4ce03f74-98a8-4242-b923-b60ae71d9b1c

@google-cla @codecov-commenter @github-actions #19022

4ce03f74-98a8-4242-b923-b60ae71d9b1c

Sent with GitHawk

@dugujiujian1999
Copy link
Contributor

dugujiujian1999 commented Jan 5, 2024

@AlvaroMaza Hello! I have a pr right now. And I am changing this file too. Would you be comfortable if I added you as a Coauthor (Co-authored-by) to acknowledge your contributions and incorporate your patches? Your expertise would be greatly valued.
This is my patch in a test branch:
dugujiujian1999@4277539
And this is my pr:
#19020

@fchollet
Copy link
Member

fchollet commented Jan 5, 2024

Sorry for the delay in merging, this was pending due to a failing test. Re-running now.

@dugujiujian1999
Copy link
Contributor

Looks like the import breaks something.

@dugujiujian1999
Copy link
Contributor

dugujiujian1999 commented Jan 6, 2024

I believe it would be beneficial to reconsider the import method. Here's the suggested approach

if backend.backend() == "tensorflow":
    from keras.utils.module_utils import tensorflow as tf

    print("Using TensorFlow backend.")
    print("TensorFlow version:", tf.__version__)
    print(tf.sparse.from_dense(np.array([[-1.0, 0.2, 0.7, 1.2]])))

@AlvaroMaza
Copy link
Contributor Author

@AlvaroMaza Hello! I have a pr right now. And I am changing this file too. Would you be comfortable if I added you as a Coauthor (Co-authored-by) to acknowledge your contributions and incorporate your patches? Your expertise would be greatly valued. This is my patch in a test branch: dugujiujian1999@4277539 And this is my pr: #19020

Yes of course! No problem at all!

@dugujiujian1999
Copy link
Contributor

@AlvaroMaza Thank you.
I've integrated the patch you provided and subsequently opened a new pr here. FYI: #19029

@sachinprasadhs
Copy link
Collaborator

@AlvaroMaza, Looks like there is some conflict with the branch you are working on, could you please resolve it.

@fchollet
Copy link
Member

Closing inactive stale PR -- feel free to reopen a new one.

@fchollet fchollet closed this Mar 22, 2024
@google-ml-butler google-ml-butler bot removed ready to pull Ready to be merged into the codebase kokoro:force-run labels Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed/Rejected
Development

Successfully merging this pull request may close these issues.

8 participants