ytdata io: use data_file.start and .end index range #4595
Conversation
| @@ -308,12 +316,14 @@ def _read_particle_fields(self, chunks, ptf, selector): | |||
| yield (ptype, field), data | |||
|
|
|||
|
|
|||
| def _get_position_array(ptype, f, ax): | |||
| def _get_position_array(ptype, f, ax, index_mask=None): | |||
| if index_mask is None: | |||
There was a problem hiding this comment.
Are slice objects mutable ? I couldn't find a reference to support it (or its contrary).
There was a problem hiding this comment.
I don't actually know either... If you try to modify attributes after instantiating you get errors:
a = slice(0, 10)
a.start = 1
raises AttributeError: readonly attribute
so immutable? maybe?
I could instead pass in the integer start/end indices here.
There was a problem hiding this comment.
They aren't, but this follows the idiom we've used in the past.
>>> a = slice(None)
>>> a.start = 1
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: readonly attributeThere was a problem hiding this comment.
If slices are immutable (which seems to be the case) I would prefer the default value tu be a slice instead of None. One way to know for sure is to call ˋhash(slice(None))` (hashability implies immutability)
There was a problem hiding this comment.
I would prefer the default value tu be a slice instead of None
I actually started with def _get_position_array(ptype, f, ax, index_mask=slice(None)): but one of the linters raised an objection with calling a function within an argument definition.
There was a problem hiding this comment.
ruff didn't like it
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1
yt/frontends/ytdata/io.py:319:50: B008 Do not perform function call `slice` in argument defaults
There was a problem hiding this comment.
Leave it. I'd prefer we not change the linter and violate idioms.
|
Confirmed that the fix does indeed fix the reported issue (with the full data, not just my reproducer). @neutrinoceros did you have any other comments? If not, good to merge I think! |
|
No further questions, thanks for your work ! |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
@chrishavlin I won't be able to perform the manual backport until tomorrow, but I'd happily do it then if it's still needed |
|
OK, tomorrow is totally fine, thanks!! |
|
The issue with backporting it is that this PR is built on top of #4579, which isn't (and shouldn't be ?) backported.
|
|
I personally don't feel strongly about backporting this so I'm fine with option 2. |
Ok, this one should actually Close #4565
The problem was that the
ytdataIOHandlerYTDataContainerHDF5was not using thedata_filestartandendindex ranges when processing each chunk, so every iteration was loading the full index range. Given that_count_particlesdoes use the proper index range, it seemed better to correct_read_particle_coordsand_read_particle_data_fileto also use the index range rather than disable chunking for particles here.Looking through the other
ytdataIOHanlders, it does seem that those should be updated as well. I'm going to open another issue for that though, as I'd like to get this one through on the faster side if possible.