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 ops.random.shuffle #907

Merged
merged 6 commits into from
Sep 19, 2023
Merged

Conversation

james77777778
Copy link
Contributor

I believe this op could be helpful when I came across the following PR:
#884 (comment)

Copy link
Contributor

@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. Yes, this is a useful op! Please also add it in keras_core/random/random.py so we can export it to the public API and document it.


self.assertFalse(
np.all(x == ops.convert_to_numpy(expected))
) # seems unlikely!
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlikely but nevertheless the test will be flaky some % of the time unless it is seeded. I recommend using a seed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. seed=0 has been added.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 86.20% and project coverage change: -3.84% ⚠️

Comparison is base (b4019bc) 83.63% compared to head (eeb31d6) 79.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #907      +/-   ##
==========================================
- Coverage   83.63%   79.80%   -3.84%     
==========================================
  Files         318      318              
  Lines       28391    28420      +29     
  Branches     5409     5413       +4     
==========================================
- Hits        23745    22680    -1065     
- Misses       3147     4281    +1134     
+ Partials     1499     1459      -40     
Flag Coverage Δ
keras_core 79.72% <86.20%> (-3.81%) ⬇️
keras_core-jax ?
keras_core-numpy 60.70% <34.48%> (-0.03%) ⬇️
keras_core-tensorflow 67.05% <41.37%> (-0.03%) ⬇️
keras_core-torch 69.41% <44.82%> (-0.03%) ⬇️

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

Files Changed Coverage Δ
keras_core/backend/jax/random.py 27.45% <33.33%> (-72.55%) ⬇️
keras_core/backend/torch/random.py 86.81% <80.00%> (-0.85%) ⬇️
keras_core/backend/numpy/random.py 100.00% <100.00%> (ø)
keras_core/backend/tensorflow/random.py 100.00% <100.00%> (ø)
keras_core/random/random.py 86.66% <100.00%> (+1.48%) ⬆️

... and 25 files with indirect coverage changes

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

@james77777778
Copy link
Contributor Author

james77777778 commented Sep 19, 2023

Please also add it in keras_core/random/random.py so we can export it to the public API and document it.

I didn't notice that there is a path for keras_core/random/random.py.

The function and corresponding docstring have been added.

EDITED:
I believe the CI failure is unrelated to this PR, and I have no idea about it.

Copy link
Contributor

@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 for the contribution! The test failure is unrelated indeed, just a flake.

@fchollet fchollet merged commit e1e207d into keras-team:main Sep 19, 2023
6 of 8 checks passed
@james77777778 james77777778 deleted the add-random-shuffle branch September 19, 2023 03:46
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.

2 participants