reduce code duplication in IOHandler _read_particle_coords and _read_particle_fields #4597
Conversation
missed one fix ahf modification
chrishavlin
added
index: particle
refactor
| return data_files | ||
|
|
||
| def _sorted_chunk_iterator(self, chunks): | ||
| data_files = self._get_data_files(chunks) |
There was a problem hiding this comment.
I think we should explicitly turn chunks into a list here, unless it has been elsewhere higher up the stack; recently we exhausted an iterator and ran into issues. (Maybe with nc_cm1?) Also, we sort with x.start and x.filename but not all frontends did that -- does x.start have a default value of 0 or something constant that we can rely on?
There was a problem hiding this comment.
Ya, _get_data_files turns chunks into a list.
Will check on how reliable the start attribute is.
There was a problem hiding this comment.
Oh and I left the front ends that do not sort by both filename and start intact. They override this method so that they are unchanged
There was a problem hiding this comment.
OK, so ParticleFile.start does initialize to 0 if it's not provided (within ParticleFile.__init__ (link to relevant code).
But in any case, the changes in this PR do not change any behavior -- only the frontends that were sorting by filename and start are using this function. Those that were not already doing that either override this method or don't use this method at all.
| @@ -194,6 +185,10 @@ def members(self, ihalo): | |||
| members = fpu.read_attrs(todo.pop(0))["particle_identities"] | |||
| return members | |||
|
|
|||
| def _sorted_chunk_iterator(self, chunks): | |||
| data_files = self._get_data_files(chunks) | |||
| yield from sorted(data_files, key=attrgetter("filename")) | |||
There was a problem hiding this comment.
@matthewturk just pointing out an example where I added an override of _sorted_chunk_iterator so that the frontend continues only sorting by filename. The change in ahf is similar.
| for chunk in chunks: | ||
| for obj in chunk.objs: | ||
| data_files.update(obj.data_files) | ||
| data_files = self._get_data_files(chunks) | ||
| assert len(data_files) == 1 | ||
| for _data_file in sorted(data_files, key=lambda x: (x.filename, x.start)): |
There was a problem hiding this comment.
just a note: I did not use the _sorted_chunk_iterator here because I wanted to keep the assert len(data_files) == 1 line above.
There was a problem hiding this comment.
Glad to see this merged if @matthewturk is still happy with it.
This PR reduces duplicated code related to iterating over
data_filechunks in_read_particle_coordsand_read_particle_fieldsacross IOHandlers. Shouldn't affect any behavior...