-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Adds a dataset that can be read and written lazily #5344
Conversation
This does not work yet. I'm still working on supporting classes.
I think I can simplify this a lot. |
else: | ||
return None | ||
elif isinstance(i, slice): | ||
from allennlp.tango.dataloader import ShuffledSequence |
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.
Is it necessary to put this import here? I'm afraid this would add non-negligible runtime overhead each time this path is reached.
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 made it a global import.
if isinstance(i, int): | ||
current_length = len(self) | ||
if i < 0: | ||
i %= current_length | ||
self.table[str(i)] = value | ||
self.table["_len"] = max(i, current_length) | ||
self.table.commit() | ||
else: | ||
raise TypeError(f"list indices must be integers, not {i.__class__.__name__}") |
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.
Why can't we handle slices here?
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.
You can't set slices. You can in numpy, but not in Python list
.
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.
But can't you just go through one-by-one?
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 guess you can? I tried to do whatever list
does. Numpy can do many things that list
can't, and I didn't implement those either.
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.
Ok, I don't see a huge need for it, so 🤷♂️
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.
LGTM!
No description provided.