-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: reintroduce Content documentation from v1 reST files. #2231
Conversation
Codecov Report
Additional details and impacted files
|
class BitMaskedArray(Content): | ||
def __init__(self, mask, content, valid_when, length, lsb_order): | ||
assert isinstance(mask, IndexU8) | ||
assert isinstance(content, Content) | ||
assert isinstance(valid_when, bool) | ||
assert isinstance(length, int) and length >= 0 | ||
assert isinstance(lsb_order, bool) | ||
assert len(mask) <= len(content) | ||
self.mask = mask | ||
self.content = content | ||
self.valid_when = valid_when | ||
self.length = length | ||
self.lsb_order = lsb_order | ||
|
||
def __len__(self): | ||
return self.length | ||
|
||
def __getitem__(self, where): | ||
if isinstance(where, int): | ||
assert 0 <= where < len(self) | ||
if self.lsb_order: | ||
bit = bool(self.mask[where // 8] & (1 << (where % 8))) | ||
else: | ||
bit = bool(self.mask[where // 8] & (128 >> (where % 8))) | ||
if bit == self.valid_when: | ||
return self.content[where] | ||
else: | ||
return None | ||
elif isinstance(where, slice) and where.step is None: | ||
# In general, slices must convert BitMaskedArray to ByteMaskedArray. | ||
bytemask = np.unpackbits( | ||
self.mask, bitorder=("little" if self.lsb_order else "big") | ||
).view(bool) | ||
return ByteMaskedArray( | ||
bytemask[where.start : where.stop], | ||
self.content[where.start : where.stop], | ||
valid_when=self.valid_when, | ||
) | ||
elif isinstance(where, str): | ||
return BitMaskedArray( | ||
self.mask, | ||
self.content[where], | ||
valid_when=self.valid_when, | ||
length=self.length, | ||
lsb_order=self.lsb_order, | ||
) | ||
else: | ||
raise AssertionError(where) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking through the v1 docs, I (personally) didn't find these code excerpts that helpful; they're not an exact copy of the source, after all. I'd be tempted to remove them in favour of the prose that we already have (perhaps addition descriptions). Do you think these are more help than hindrance, i.e. we should keep them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence. At first, I wasn't going to include them, but then concluded that they're useful because the constraints on the constructor arguments are not documented elsewhere (types help with that, but there are also value constraints), and the way getitem computes an item is not documented elsewhere (which is the main thing).
At minimum, saying what __getitem__(self, where: int)
returns is a large part of saying what a given layout node is.
In these listings, I removed the less relevant parts.
Maybe we could ask @ivirshup if they're useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it did feel like these are now smaller. If we removed the code, we would want to verbally explain some of these constraints.
I'm going to be pragmatic: we don't have the time and energy to split these kind of hairs. Code is good for expressing constraints, and you've slimmed these down to pretty much just that.
Unless @ivirshup feels strongly, I support your reasoning to keep them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll set this to auto-merge, and we can always remove them if prompted. (And then we'd have to find another way to express the constraints and __getitem__
.)
Co-authored-by: Angus Hollands <goosey15@gmail.com>
Thanks for copying this over Jim. It looks exactly like what was asked for in the motivating issue :) |
Specifically,