Repository navigation
[BugFix] Reject a names list whose length is not batch_dims - #2653
Open
YeonwooSung wants to merge 2 commits into
Open
YeonwooSung wants to merge 2 commits into
YeonwooSung wants to merge 2 commits into
Conversation
A list with exactly batch_dims None entries was accepted and every name was dropped. Lock the length check, the all-None clear, and the lazy stack that forwards the extra entry to its members.
_set_names cleared names whenever a list held exactly batch_dims None entries, and it did that before checking the length. A longer list then dropped every name, including when a lazy stack forwarded the extra entry to its members. Check the length first. An all-None list of the right length still clears names.
This branch has not been deployed
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.
Fixes #2600
TensorDictBase._set_namescountedNoneentries and, when that count equalledbatch_dims, cleared the names and returned before checking thatlen(value) == batch_dims. A list one entry too long was therefore accepted whenever it held exactlybatch_dimsNones, and every name was dropped:The same shortcut ran for a lazy stack.
s.names = ["s", "t", None]on members of batch[3]forwards["t", None]to each member, which has exactly oneNone. The member cleared its names and"t"was dropped. Checking the length in the base method rejects that list before any name is written on the member, so the members are not left half-updated.The length check now comes first and raises the existing
ValueError(the length of the dimension names must equate the tensordict batch_dims attribute). Names that were already set are left unchanged. An all-Nonelist of the right length still clears names. A right-length mix of strings andNones still assigns.This is a silent data loss, so it raises now. It is not a scheduled default change.