Dense: validate units argument #21902
Conversation
Summary of ChangesHello @RohitYandigeri, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds input validation for the units argument in the Dense layer, which is a great improvement for robustness and user experience. The changes ensure that units is a positive integer, raising a clear ValueError for invalid inputs. The accompanying unit tests cover a good range of invalid cases.
My review includes two suggestions:
- In
keras/src/layers/core/dense.py, I recommend a more robust validation check forunitsthat correctly handles numpy integer types, which are common in the ecosystem. The currentisinstance(units, int)is a bit too strict. - In
keras/src/layers/core/dense_test.py, I suggest parameterizing the new test for invalidunitsto make it more concise and easier to maintain.
Overall, this is a valuable and well-implemented change.
| if not isinstance(units, int) or units <= 0: | ||
| raise ValueError( | ||
| "Received an invalid value for `units`, expected a positive integer. " | ||
| f"Received: units={units}" | ||
| ) |
There was a problem hiding this comment.
The validation isinstance(units, int) is a bit too strict as it will reject numpy integer types (e.g., numpy.int64), which are commonly used in the scientific Python ecosystem. This can lead to unexpected errors for users.
A more robust approach is to check if units is integer-like by trying to convert it to an int and checking for equality. This correctly handles integers, numpy integers, and integer-like floats (e.g., 2.0), while rejecting non-integer floats and other types like strings.
| if not isinstance(units, int) or units <= 0: | |
| raise ValueError( | |
| "Received an invalid value for `units`, expected a positive integer. " | |
| f"Received: units={units}" | |
| ) | |
| try: | |
| is_int_like = int(units) == units | |
| except (TypeError, ValueError): | |
| is_int_like = False | |
| if not is_int_like or units <= 0: | |
| raise ValueError( | |
| "Received an invalid value for `units`, expected a positive integer. " | |
| f"Received: units={units}" | |
| ) |
keras/src/layers/core/dense_test.py
Outdated
| def test_dense_invalid_units_raises(self): | ||
| with self.assertRaisesRegex(ValueError, "positive integer"): | ||
| layers.Dense(0) | ||
|
|
||
| with self.assertRaisesRegex(ValueError, "positive integer"): | ||
| layers.Dense(-3) | ||
|
|
||
| with self.assertRaisesRegex(ValueError, "positive integer"): | ||
| layers.Dense(2.5) | ||
|
|
||
| with self.assertRaisesRegex(ValueError, "positive integer"): | ||
| layers.Dense(None) | ||
|
|
||
| with self.assertRaisesRegex(ValueError, "positive integer"): | ||
| layers.Dense("64") |
There was a problem hiding this comment.
These tests for invalid units values are great. To make the test more concise and easier to extend in the future, you could use parameterized.named_parameters which is already used in this test file.
| def test_dense_invalid_units_raises(self): | |
| with self.assertRaisesRegex(ValueError, "positive integer"): | |
| layers.Dense(0) | |
| with self.assertRaisesRegex(ValueError, "positive integer"): | |
| layers.Dense(-3) | |
| with self.assertRaisesRegex(ValueError, "positive integer"): | |
| layers.Dense(2.5) | |
| with self.assertRaisesRegex(ValueError, "positive integer"): | |
| layers.Dense(None) | |
| with self.assertRaisesRegex(ValueError, "positive integer"): | |
| layers.Dense("64") | |
| @parameterized.named_parameters( | |
| ("zero", 0), | |
| ("negative", -3), | |
| ("float", 2.5), | |
| ("none", None), | |
| ("string", "64"), | |
| ) | |
| def test_dense_invalid_units_raises(self, units): | |
| with self.assertRaisesRegex(ValueError, "positive integer"): | |
| layers.Dense(units) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21902 +/- ##
=======================================
Coverage 76.30% 76.30%
=======================================
Files 580 580
Lines 60029 60031 +2
Branches 9432 9433 +1
=======================================
+ Hits 45803 45805 +2
Misses 11750 11750
Partials 2476 2476
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It looks like the test suite completed successfully, but the job exited |
hertschuh
left a comment
hertschuh
left a comment
There was a problem hiding this comment.
Thanks you for the PR!
keras/src/layers/core/dense.py
Outdated
| not isinstance(units, numbers.Integral) | ||
| or isinstance(units, bool) |
There was a problem hiding this comment.
Simply replace this with:
if not isinstance(units, int) or units <= 0:There was a problem hiding this comment.
Thanks for the clarification!
I’ve updated the validation to enforce a strict int requirement for units and removed the broader numbers.Integral check accordingly. Tests continue to cover invalid inputs.
Locally, the api-gen pre-commit hook reports modified generated files, but I’ve not included those outputs in the commit, following the usual contributor workflow. Happy to regenerate or adjust if you’d prefer otherwise.
Please let me know if you’d like any further changes.
There was a problem hiding this comment.
The "code format" check is failing. I believe it's the extra empty line you added line 104.
But the pre-commit hook should just fix that, just do git add keras after it fails.
|
@hertschuh Thanks for the pointer! I reran the pre-commit hooks and staged the formatting changes as suggested. The code format check is now passing. Please let me know if there’s anything else to adjust. |
google-ml-butler
bot
added
kokoro:force-run
ready to pull
4 participants
This PR adds stricter validation for the
unitsargument inkeras.layers.Dense.Previously, non-integer or non-positive values could be accepted and either be
silently truncated or fail later with unclear errors. This change ensures that
unitsmust be a positive integer and raises a clear ValueError otherwise.Includes unit tests covering invalid
unitsvalues.This follows TensorFlow review feedback indicating that Dense API validation
should live in the Keras repository.