Allow two (same/different) Batch objs to be tested for equality #1098
Conversation
This file contains 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
MischaPanch
requested changes
Apr 5, 2024
* Introduce a new test class named `TestBatchEquality`. * Got rid of comments and assert messages as the test method names are expressive enough.
* Restrict to Batch in method
* Add tests for `Batch.to_dict()`
* Add deepdiff library (for testing equality of dicts in the tests mentioned above)
* Update poetry.toml and poetry.lock
* Note: `Batch.to_numpy()` should be extended to support also a non in-place operation.
MischaPanch
reviewed
Apr 12, 2024
tianshou/data/batch.py
Outdated
| if not isinstance(other, self.__class__): | ||
| return False | ||
|
|
||
| self.to_numpy() |
There was a problem hiding this comment.
This modifies self and other inplace on a supposedly harmless equality check - this should never happen.
Starting a new thread about the questions you raised regarding to_numpy.
- For other methods we have the convention that things ending in
_are inplace. So it should really be thatto_numpy_operates inplace and returns nothing, whereasto_numpy()doesn't change the original and returns a new batch. That's a breaking change, so you'd need to check all the places whereto_numpyis currently being used. - I think the name
to_numpyis ok for the moment, since all values of batch are either batches, tensors or arrays, right? Then it's clear thatto_numpywould turn all tensors into arrays.
There was a problem hiding this comment.
Batch.to_torch() has same problem because it is also in-place. We can change this in another PR?
There was a problem hiding this comment.
Yes, thanks for bringing it up! We should generally reduce the number of inplace operations as much as possible
MischaPanch
reviewed
Apr 12, 2024
* Breaking change: Previous in-place `Batch.to_numpy` is now `Batch.to_numpy_` (following naming convention of other in-place methods). * Update places where in-place was expected * Add tests for both to_numpy/to_numpy_
MischaPanch
reviewed
Apr 16, 2024
MischaPanch
approved these changes
Apr 16, 2024
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.
Closes: #1086
Api Extensions
to_numpy_. Allow two (same/different) Batch objs to be tested for equality #1098to_dictin Batch supports also non-recursive conversion. Allow two (same/different) Batch objs to be tested for equality #1098__eq__now implemented, semantic equality check of batches is now possible. Allow two (same/different) Batch objs to be tested for equality #1098Breaking Changes
to_numpyindata.utils.batch.Batchis not in-place anymore. Instead, a new methodto_numpy_does the conversion in-place. Allow two (same/different) Batch objs to be tested for equality #1098