add simstore table access via slice#1050
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1050 +/- ##
==========================================
- Coverage 81.57% 81.57% -0.01%
==========================================
Files 140 140
Lines 15421 15421
==========================================
- Hits 12580 12579 -1
- Misses 2841 2842 +1
Continue to review full report at Codecov.
|
dwhswenson
left a comment
There was a problem hiding this comment.
Thanks -- I've also noticed this as a minor annoyance (usually I'm too busy with whatever I'm working on and just use list(storage.steps)[start:end], because hacks like that are obvious to me -- that kind of hack is often NOT obvious to our users, and we shouldn't expect them to know it -- so this is a very useful improvement.)
The only required change is to fix a typo in an error message. Beyond that, I hope you'll explain why one part of the code works (because I don't get it!), and other things are fixes for a mistake I made and some general "let's agree on some code style that isn't officially defined in PEP-8" stuff.
Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>
Yeah, that would be a solution if I actually could load all the steps (100_000) on my local machine in a reasonable time, I can't 😆 My hack around was to write something like stop = 100:
steps = []
for i, e in storage.steps:
steps.append(e)
if i > stop:
breakWhich was not great xD With d1c8609, I actually test the correct code path. I also tested that this works (and hits the code) on the "Simstore and CLI" minitutorial. |
|
@dwhswenson please have another look, thanks for catching the wrong code! |
|
also, I would like to know if:
Is a reasonable assumption to enforce |
I think this should be the case, although it looks like SimStore isn't raising an error if you try to save an object with a non-string name. The PR coming to make this explicitly required. |
dwhswenson
left a comment
There was a problem hiding this comment.
One more tiny change for readability (sorry to miss this one before). Otherwise, looks good.
I was debating whether to do isinstance(item, slice) or type(item) is slice: I agree that there's no distinction, since slice is final (can't be subclassed), but I'd also probably look for this code by grepping "isinstance". Anyway, either one is fine.
Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>
dwhswenson
left a comment
There was a problem hiding this comment.
Thanks! Will merge this one now.
Today, I got annoyed today that I could not do
steps = storage.steps[:100]in some test code using simstore.This adds handling of slices in the
__getitem__logic.It behaves as a
listwould do (so an empty list on an empty slice, a list with a single item for slices like[4:5], a single item withintand aTypeErrorif it is not anintorslice)It should also leverage batched loading, if I understood the code correctly
One assumption that I added is that the
nameof objects can only be strings (for_name_to_uuid), I don't know if this is currently an assumption/requirement, but all tests seem to pass on this.This code is
python > .3.6(f strings) and the tests arepython > 3.7(leverages dict order)