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

Solving issue #20221 #20237

Merged

Conversation

sanskarmodi8
Copy link
Contributor

Enhanced the compute_output_shape method by adding validation for input shape types and axis bounds. The method now ensures that input_shape is either a list or tuple, and that all axis values are within the valid range of input dimensions. This provides clearer error handling and prevents potential issues during shape computation.

Reason for the Change: Previously, the compute_output_shape method did not validate the input shape type or check if the specified axes were within bounds. This could lead to ambiguous or delayed errors during dynamic execution when invalid input shapes or out-of-bound axes were used, leading to confusion for the user.
The updated validation improves robustness by catching these issues earlier in the process, providing more informative error messages to the user.

Hence solving the issue #20221

… passed to compute_output_shape func in UnitNormalization Layer
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.38%. Comparing base (7496e18) to head (a370804).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...ras/src/layers/normalization/unit_normalization.py 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #20237   +/-   ##
=======================================
  Coverage   79.38%   79.38%           
=======================================
  Files         501      501           
  Lines       47483    47503   +20     
  Branches     8718     8725    +7     
=======================================
+ Hits        37694    37712   +18     
- Misses       8025     8026    +1     
- Partials     1764     1765    +1     
Flag Coverage Δ
keras 79.24% <66.66%> (+<0.01%) ⬆️
keras-jax 62.51% <66.66%> (+<0.01%) ⬆️
keras-numpy 57.69% <0.00%> (-0.01%) ⬇️
keras-tensorflow 63.89% <66.66%> (+<0.01%) ⬆️
keras-torch 62.55% <66.66%> (+<0.01%) ⬆️

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.

@sanskarmodi8
Copy link
Contributor Author

There are similar issues for different layers like BatchNormalization, LayerNormalization, etc. So should I add validation checks similarily in those and commit into this PR itself or should I make seperate one for each of them?

@ghsanti
Copy link
Contributor

ghsanti commented Sep 9, 2024

unsure if all norms can be / should be bundled together; if they do, it may be worth adding it here https://github.com/keras-team/keras/blob/master/keras/src/ops/operation_utils.py

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Sep 9, 2024
@fchollet fchollet merged commit 7b4a78c into keras-team:master Sep 9, 2024
6 of 9 checks passed
@google-ml-butler google-ml-butler bot removed awaiting review ready to pull Ready to be merged into the codebase labels Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

6 participants