feat!: re-introduce unknown-length
#2229
Merged
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
unknown-lengthunknown-length
agoose77
commented
Feb 9, 2023
| @@ -61,7 +62,7 @@ def _to_dict_part(self, verbose, toplevel): | |||
| return self._to_dict_extra( | |||
| { | |||
| "class": "RegularArray", | |||
| "size": self._size, | |||
| "size": None if self._size is unknown_length else self._size, | |||
There was a problem hiding this comment.
Here we encode unknown_length as None
| @@ -54,7 +53,7 @@ def from_dict(input: dict) -> Form: | |||
| elif input["class"] == "RegularArray": | |||
| return ak.forms.RegularForm( | |||
| content=from_dict(input["content"]), | |||
| size=input["size"], | |||
| size=unknown_length if input["size"] is None else input["size"], | |||
There was a problem hiding this comment.
Here we decode None to mean unknown_length
There was a problem hiding this comment.
BitMaskedArray has length metadata that can now be unknown_length, but I guess it's not a part of the BitMaskedForm the way that size is for RegularForm. I had thought that there might be another place to do this, but apparently not.
Codecov Report
Additional details and impacted files
|
jpivarski
approved these changes
Feb 9, 2023
| @@ -54,7 +53,7 @@ def from_dict(input: dict) -> Form: | |||
| elif input["class"] == "RegularArray": | |||
| return ak.forms.RegularForm( | |||
| content=from_dict(input["content"]), | |||
| size=input["size"], | |||
| size=unknown_length if input["size"] is None else input["size"], | |||
There was a problem hiding this comment.
BitMaskedArray has length metadata that can now be unknown_length, but I guess it's not a part of the BitMaskedForm the way that size is for RegularForm. I had thought that there might be another place to do this, but apparently not.
Comment on lines
-488
to
+489
| for x1 in [True, False, None]: | ||
| for x2 in [True, False, None]: | ||
| for x3 in [True, False, None]: | ||
| for x4 in [True, False, None]: | ||
| for x5 in [True, False, None]: | ||
| mask = [x1, x2, x3, x4, x5] | ||
| expected = [ | ||
| m if m is None else x | ||
| for x, m in zip(data, mask) | ||
| if m is not False | ||
| ] | ||
| array2 = ak.highlevel.Array(mask, check_valid=True).layout | ||
| assert to_list(array[array2]) == expected | ||
| assert array.to_typetracer()[array2].form == array[array2].form | ||
| for mask in itertools.repeat([True, False, None], 5): |
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.
TL;DR
Noneas "unknown length" token withunknown_lengthsingleton.nplike.add_shape_item(length, x)withlength + xTL
The rationale for using
Noneas a token for an unknown length was weak, but was ultimately due to the following factors:Nonefor unknown dimension lengthsNoneeasily serialises to JSONNoneis a natural singletonSome of this was discussed in #2220 (comment)
However, a significant problem with using
Noneis that it is also a meaningful index value, and we would be unable to disambiguate betweenarray[None]andarray[array.layout.length]for typetracer arrays. Whilst this would only affect L2 typetracer users, it's enough to change the balance.Thereafter, there are arguments to be made in favour of using
nplike.XXX_shape_itemfunctions to operate upon lengths vs a rich object that implements magic methods. I preferred the former, due to explicitness and the ability to do e.g. validation on known lengths (such as ensuring that they are positive). However, code is less easy to read from an expression perspective. @jpivarski and I discussed this, and we settled on re-introducing the object-oriented API.We will maintain a strong distinction between an
index: int | ArrayLikeand alength: int | type[unknown_length], to ensure that unknown values map correctly between spaces.This reverts the inability to set
Noneas length values inRecordArray.