[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

validate argument axis of tf.experimental.numpy.stack #60727

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

validate argument axis of tf.experimental.numpy.stack #60727

wants to merge 2 commits into from

Conversation

SuryanarayanaY
Copy link
Collaborator
@SuryanarayanaY SuryanarayanaY commented May 30, 2023 •
edited
Loading

AT present tf.experimental.numpy.stack not validating the values for axis argument. If we pass out of bound values to the axis still it is not rising the Error. Hence I am proposing amendments to validate the axis argument and raise the Error in case of out of bound axis.

Please find the attached gist for explaining the problem and also with proposed amendments to the code it works fine and raised intended error when invalid axis arguments provided.

Fixes #55217

@SuryanarayanaY
validate tf.experimental.numpy.stack axis argument
ad5a72f 
AT present tf.experimental.numpy.stack not validating the axis argument. If we pass out of bound values to the axis still it is not rising the Error. Hence I am proposing amendment to validate the axis argument and raise the Error in case of out of bound axis.

Please find the attached gist for explaining the problem and also with proposed amendments to the code it works fine and raised intended error when invalid axis arguments provided.
@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label May 30, 2023
@google-ml-butler google-ml-butler bot requested a review from cantonios May 30, 2023 13:03
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label May 30, 2023
@cantonios
Copy link
Contributor

It is raising an error. Even in your gist you provide in this description. So what's the issue?

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Jun 1, 2023
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jun 1, 2023
@gbaned gbaned added the comp:ops OPs related issues label Jun 1, 2023
@SuryanarayanaY
Copy link
Collaborator Author

It is raising an error. Even in your gist you provide in this description. So what's the issue?

Sorry you might have gone through the tf.stack. I have deleted that part in gist and updated it.

Its not raising the error for tensorflow.experimental.numpy.stack. Please check the below code in gist for and its not raising the error.

import tensorflow.experimental.numpy as tnp
tnp.experimental_enable_numpy_behavior()
x = tf.random.uniform([5])
tnp.stack(x, axis=-2) # Passes without any error

Thanks!

@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label Jun 2, 2023
@cantonios
Copy link
Contributor

That's not throwing an error because the axis is valid: it can be in the range [0, 1], and negative values work backwards from the end, so -2 corresponds to axis=0 and -1 corresponds to axis=1 in this case. Your example fails if you pass axis=-3.

@gbaned
Copy link
Contributor
gbaned commented Jun 8, 2023

Hi @SuryanarayanaY Can you please check @cantonios's comments ? Thank you!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Jun 8, 2023
@SuryanarayanaY
Copy link
Collaborator Author

For earlier case the explanation holds right. I tested with new code.Please refer below code.

import tensorflow.experimental.numpy as tnp
tnp.experimental_enable_numpy_behavior()
x = tf.random.uniform([5,3])
tnp.stack(x, axis=-4) # passes without error
tnp.stack(x, axis=-3) # passes without error

Whereas with Numpy it raises error.

import numpy as np
x_n = np.random.randn(5, 3)
print(np.shape(x_n))
np.stack(x_n, axis=-4) # raises error
np.stack(x_n, axis=-3) # raises error

Please refer the new gist attached here and the proposed code change might resolve this.

@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label Jun 9, 2023
@gbaned gbaned added the awaiting review Pull request awaiting review label Jun 19, 2023
@gbaned
Copy link
Contributor
gbaned commented Jun 21, 2023

Hi @cantonios Can you please review this PR ? Thank you!

cantonios
cantonios requested changes Jun 21, 2023
View reviewed changes
tensorflow/python/ops/numpy_ops/np_array_ops.py Outdated
@@ -1066,6 +1066,10 @@ def stack(arrays, axis=0): # pylint: disable=missing-function-docstring
arrays = asarray(arrays)
if axis == 0:
return arrays
elif axis < -array_ops.rank(arrays) or axis >= array_ops.rank(arrays):
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is actually in swapaxes, which doesn't properly check that the axis is in the valid range.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cantonios

I have tested implementing range validation in swapaxes and it seems working fine. Attached gist for reference.

Validation Code and location can be found below.

def swapaxes(a, axis1, axis2):  # pylint: disable=missing-docstring
  a = asarray(a) #
  def adjust_axes(axes, rank):
    def f(x):
      if isinstance(x, int):
        if x < 0:
          x = x + rank
      else:
        x = array_ops.where_v2(x < 0, np_utils.add(x, a_rank), x)
      return x
    return nest.map_structure(f, axes)

  if axis2 < -array_ops.rank(a) or axis2 >= array_ops.rank(a):
    raise ValueError(
        f"Argument `axis` = {axis2} not in range "
                      f"[{-array_ops.rank(a)}, {array_ops.rank(a)})")

Is it good to go implementing the validation code in swapaxes instead of stack ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The check should be in swapaxes, which has the same issue. It currently assumes that axis is always in the range [-rank, rank).

Copy link
Contributor

Choose a reason for hiding this comment

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

Still waiting for the check to be made in swapaxes

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Jun 21, 2023
@gbaned
Copy link
Contributor
gbaned commented Jul 3, 2023

Hi @SuryanarayanaY Can you please check @cantonios's comments and keep us posted ? Thank you!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Jul 3, 2023
@SuryanarayanaY
Copy link
Collaborator Author

Requested for review

@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label Jul 14, 2023
@gbaned gbaned added awaiting review Pull request awaiting review and removed awaiting review Pull request awaiting review labels Jul 14, 2023
@gbaned gbaned added the awaiting review Pull request awaiting review label Aug 3, 2023
@gbaned
Copy link
Contributor
gbaned commented Aug 3, 2023

Hi @cantonios Can you please review this PR ? Thank you!

@gbaned
Copy link
Contributor
gbaned commented Jan 15, 2024

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

2 similar comments
@gbaned
Copy link
Contributor
gbaned commented Jan 24, 2024

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

@gbaned
Copy link
Contributor
gbaned commented Feb 2, 2024

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

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Feb 2, 2024
@github-actions GitHub Actions
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 This label marks the issue/pr stale - to be closed automatically if no activity label Feb 17, 2024
@gbaned
Copy link
Contributor
gbaned commented Feb 20, 2024

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

@google-ml-butler google-ml-butler bot removed stale This label marks the issue/pr stale - to be closed automatically if no activity stat:awaiting response Status - Awaiting response from author labels Feb 20, 2024
@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Feb 20, 2024
@github-actions GitHub Actions
Copy link
github-actions bot commented Mar 6, 2024

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 This label marks the issue/pr stale - to be closed automatically if no activity label Mar 6, 2024
@gbaned
Copy link
Contributor
gbaned commented Mar 6, 2024

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

@google-ml-butler google-ml-butler bot removed stale This label marks the issue/pr stale - to be closed automatically if no activity stat:awaiting response Status - Awaiting response from author labels Mar 6, 2024
@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Mar 8, 2024
@github-actions GitHub Actions
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 This label marks the issue/pr stale - to be closed automatically if no activity label Mar 23, 2024
@gbaned
Copy link
Contributor
gbaned commented Apr 2, 2024

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

@google-ml-butler google-ml-butler bot removed stale This label marks the issue/pr stale - to be closed automatically if no activity stat:awaiting response Status - Awaiting response from author labels Apr 2, 2024
@gbaned
Copy link
Contributor
gbaned commented Apr 25, 2024

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

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Apr 26, 2024
@gbaned
Copy link
Contributor
gbaned commented May 10, 2024

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

@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label May 10, 2024
@gbaned
Copy link
Contributor
gbaned commented May 18, 2024

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

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label May 29, 2024
@gbaned
Copy link
Contributor
gbaned commented Jun 6, 2024

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

@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label Jun 6, 2024
@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cantonios cantonios cantonios approved these changes

Assignees

@gbaned gbaned

Labels
comp:ops OPs related issues size:XS CL Change Size: Extra Small stat:awaiting response Status - Awaiting response from author
Projects
PR Queue
  
Approved by Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tf.experimental.numpy.stack should check out-of-bound axis
4 participants
@SuryanarayanaY @cantonios @gbaned @kokoro-team