-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Debugger Plugin] Fix a bug in handling string scalar #1131
Conversation
Please note that the Travis test failure is unrelated to this PR. |
Gentle ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits. Thank you for letting users be able to view scalar string values!
Rebasing to HEAD will do away with the build error. #1132 fixed the build.
self.assertEqual([['corge']], data) | ||
|
||
def testArrayViewOnScalarString(self): | ||
# Construt a numpy array that corresponds to a TensorFlow string tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe s/array/scalar to make this comment more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
self.assertEqual([['foo', 'bar'], ['baz', 'corge']], data) | ||
|
||
def testArrayViewSlicingStringTensorToScalar(self): | ||
# Construt a numpy array that corresponds to a TensorFlow string tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/Construt/Construct here and elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -82,25 +82,42 @@ def array_view(array, slicing=None, mapping=None): | |||
""" | |||
|
|||
dtype = str(array.dtype) | |||
# String-type TensorFlow Tensors are represented as object-type arrays in | |||
# numpy. We map the type name back to 'string' for clarity. | |||
if dtype == 'object': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, is this the only way to identify a numpy array that represents a TensorFlow string tensor? Do the methods in this StackOverflow post work at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a tensorflow string-type tensor, the corresponding numpy array has the dtype "object", i.e., different from a regular string-type array in numpy. This is how tensorflow is and we need to live with that.
return dtype, shape, sliced_array.tolist() | ||
|
||
if np.isscalar(sliced_array) and str(dtype) == 'string': | ||
# When a string Tensor (for which dtype is 'object') is sliced down to only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this logic exist because numpy.reshape(sliced_array, array.shape)
could separate the string into several strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in this special case is because in the case of a string scalar, sliced
array is just a Python string, i.e., not a numpy array. This is specific to the scalar case, i.e., if you have vector of strings with length > 1, you still get a numpy array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
@@ -82,25 +82,42 @@ def array_view(array, slicing=None, mapping=None): | |||
""" | |||
|
|||
dtype = str(array.dtype) | |||
# String-type TensorFlow Tensors are represented as object-type arrays in | |||
# numpy. We map the type name back to 'string' for clarity. | |||
if dtype == 'object': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a tensorflow string-type tensor, the corresponding numpy array has the dtype "object", i.e., different from a regular string-type array in numpy. This is how tensorflow is and we need to live with that.
return dtype, shape, sliced_array.tolist() | ||
|
||
if np.isscalar(sliced_array) and str(dtype) == 'string': | ||
# When a string Tensor (for which dtype is 'object') is sliced down to only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in this special case is because in the case of a string scalar, sliced
array is just a Python string, i.e., not a numpy array. This is specific to the scalar case, i.e., if you have vector of strings with length > 1, you still get a numpy array.
self.assertEqual([['corge']], data) | ||
|
||
def testArrayViewOnScalarString(self): | ||
# Construt a numpy array that corresponds to a TensorFlow string tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
self.assertEqual([['foo', 'bar'], ['baz', 'corge']], data) | ||
|
||
def testArrayViewSlicingStringTensorToScalar(self): | ||
# Construt a numpy array that corresponds to a TensorFlow string tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Related to: #1115 After this change, visualization of string-type tensors is still not fully working, but at least the user can slice a string tensor to a single element (i.e., a string scalar) and see its value.
Related to: #1115
After this change, visualization of string-type tensors is
still not fully working, but at least the user can slice a
string tensor to a single element (i.e., a string scalar) and
see its value.