Some tests misusing assertTrue for comparisons fix #16771
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
|
tests are failing now because the comparisons are actually being performed by the tests. Before the tests were just checking if the first argument was truthy, because that's what I'm just a static analysis bot (boop beep) so I'm not sure if the comparisons in the tests are wrong and therefore the tests need updating, or if the logic under test need fixing. Can someone please confirm. |
|
Hey @code-review-doctor, It seems like you uncovered some bugs in the testing - thanks a lot! Do you want to fix them or should I go ahead and fix them? |
|
@patrickvonplaten glad to be of service :) If you could go ahead and fix them that would be great. My "thing" is running static analysis checkers to detect common easily missed mistakes on hundreds of open source projects and then letting them know so they can fixed properly :) FYI and FWIW I have GitHub integration so I can review your PRs to prevent problems like this in future :) |
…to fix-avoid-misusing-assert-true
| @@ -299,6 +299,10 @@ def convert_tokens_to_string( | |||
| if output_word_offsets: | |||
| word_offsets = self._get_word_offsets(char_offsets, self.replace_word_delimiter_char) | |||
|
|
|||
| # don't output chars if not set to True | |||
There was a problem hiding this comment.
The test uncovered a bug here, which is fixed by setting char_offsets to None if it shouldn't be returned.
| @@ -416,11 +416,11 @@ def test_diagonalize(self): | |||
|
|
|||
| def test_pad_and_transpose_last_two_dims(self): | |||
| hidden_states = self._get_hidden_states() | |||
| self.assertTrue(hidden_states.shape, (1, 8, 4)) | |||
| self.assertEqual(hidden_states.shape, (1, 4, 8)) | |||
There was a problem hiding this comment.
This was recently correct by @ydshieh if I remember correctly
There was a problem hiding this comment.
[Just to add some context]
The PR #15537 didn't touch the details like LongformerSelfAttention._pad_and_transpose_last_two_dims.
In the simplest words, that PR just performed unpadding in the final outputs in (TF)LongformerEncoder.
Thanks again, @code-review-doctor !
|
Thanks a lot @code-review-doctor ! |
* Fix issue avoid-misusing-assert-true found at https://codereview.doctor * fix tests * fix tf Co-authored-by: Patrick von Platen <[email protected]>
Fixes #16770