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

Raise error when inputs and mask don't have same shape in Softmax #19736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SuryanarayanaY
Copy link
Contributor

Currently keras.layers.Softmax not having check to verify inputs and mask shape same or not. As per documnetation the mask should be A boolean mask of the same shape as inputs.

But Its generating output even though mask shape is not same as that of inputs.

Hence proposing a check to raise exception when inputs and mask don't have same shape.

Fixes #19722 .

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2024

Codecov Report

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

Project coverage is 56.30%. Comparing base (a05ac12) to head (55d34df).
Report is 195 commits behind head on master.

Files Patch % Lines
keras/src/layers/activations/softmax.py 0.00% 1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (a05ac12) and HEAD (55d34df). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (a05ac12) HEAD (55d34df)
keras 4 1
keras-torch 1 0
keras-tensorflow 1 0
keras-jax 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #19736       +/-   ##
===========================================
- Coverage   78.53%   56.30%   -22.24%     
===========================================
  Files         498      498               
  Lines       45753    45755        +2     
  Branches     8455     8456        +1     
===========================================
- Hits        35933    25762    -10171     
- Misses       8086    18402    +10316     
+ Partials     1734     1591      -143     
Flag Coverage Δ
keras 56.30% <0.00%> (-22.09%) ⬇️
keras-jax ?
keras-numpy 56.30% <0.00%> (-0.01%) ⬇️
keras-tensorflow ?
keras-torch ?

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.

@@ -50,6 +50,12 @@ def __init__(self, axis=-1, **kwargs):

def call(self, inputs, mask=None):
if mask is not None:
if mask.shape != inputs.shape:
raise ValueError(
"`mask` and `inputs` must have same shape. "
Copy link
Member

Choose a reason for hiding this comment

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

They just need to be broadcastable. For instance, if the mask has shape (b, t) and the output has shape (b, t, c) that's fine.

Copy link
Contributor Author

@SuryanarayanaY SuryanarayanaY May 23, 2024

Choose a reason for hiding this comment

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

Actually documentation states that mask should be of same shape. May be we need to add here that same shape or broadcastable shape.

mask: A boolean mask of the same shape as `inputs`. The mask
          specifies 1 to keep and 0 to mask. Defaults to `None`.

Also the first dimension of shape i.e batch_size should not be considered in shape. Because if we pass input_shape as (5, 4, 1) and mask as (5,4) we get error as "Incompatible shapes: [5,4,1] vs. [5,4]". Do we need to mention this explicitly in docs?

Also the compute_output_shape method simply returns input_shape. Suppose if I pass

x = np.random.rand(7, 1, 5)
mask = np.ones((5, 1))
l_1 = keras.layers.Softmax(axis=-1,trainable = True,dtype='float32',autocast = True)
print(l_1(x).shape) #(7, 1, 5)
print(l_1(x,mask).shape) #(7, 5,1) 
print(l_1.compute_output_shape(x.shape)) #(7, 1, 5)

Noticed output shape is different from compute_output_shape method as the mask is broadcastable here.
If I change mask shape to (1,5) instead of (5,1) then all shapes are same.

Attached gist for reference in case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @fchollet Any update on this PR? Please. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Assigned Reviewer
Development

Successfully merging this pull request may close these issues.

softmax sliently generate a wrong output when the mask has an incompatible shape
4 participants