Fix #418: add tests for padded_to_ragged2d#1068
Open
mhucka wants to merge 4 commits into
Open
Conversation
This adds some simple tests for an untested function in `tfq_utility_ops_test.py`.
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates tfq_utility_ops_test.py by correcting a typo, adding more test cases to test_padded_to_ragged, and implementing a new test for padded_to_ragged2d. Feedback indicates that the logic in test_padded_to_ragged2d contains an error in tensor slicing that only works for square matrices and suggests that the test implementation is too similar to the actual code, potentially missing bugs.
Comment on lines
+170
to
+173
| col_mask = tf.abs(tensor_arr[:, 0]) < 1.1 | ||
| masked = tf.ragged.boolean_mask(tensor_arr, col_mask) | ||
| mask = tf.abs(masked) < 1.1 | ||
| expected = tf.ragged.boolean_mask(masked, mask) |
Contributor
There was a problem hiding this comment.
The logic used to calculate the expected value in test_padded_to_ragged2d is flawed and only works for square matrices.
- Incorrect Slicing:
tensor_arr[:, 0]selects the first row of each matrix in the batch, resulting in a mask of shape[batch, columns]. When used intf.ragged.boolean_mask(tensor_arr, col_mask), it attempts to mask the second dimension (rows) oftensor_arr. This will cause a shape mismatch error if the number of rows does not equal the number of columns. To correctly mask rows based on their first element, you should usetensor_arr[:, :, 0]. - Tautological Test: The test currently replicates the implementation of the op it is testing. This means that if the op contains a logic error (which it does, as it uses the same incorrect slicing), the test will still pass, failing to catch the bug.
Note that the implementation of padded_to_ragged2d in tfq_utility_ops.py likely requires a similar fix.
Suggested change
| col_mask = tf.abs(tensor_arr[:, 0]) < 1.1 | |
| masked = tf.ragged.boolean_mask(tensor_arr, col_mask) | |
| mask = tf.abs(masked) < 1.1 | |
| expected = tf.ragged.boolean_mask(masked, mask) | |
| row_mask = tf.abs(tensor_arr[:, :, 0]) < 1.1 | |
| masked_rows = tf.ragged.boolean_mask(tensor_arr, row_mask) | |
| element_mask = tf.abs(masked_rows) < 1.1 | |
| expected = tf.ragged.boolean_mask(masked_rows, element_mask) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds some simple tests for an untested function in
tfq_utility_ops_test.py.