Move MDGeomParser into castep_outputs
#218
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
oerc0122
force-pushed
the
add-md-geom-parser
branch
4 times, most recently
from
01feeba to
2ef9b18
Compare
oerc0122
force-pushed
the
add-md-geom-parser
branch
2 times, most recently
from
f63a3ff to
2945a7c
Compare
oerc0122
force-pushed
the
add-md-geom-parser
branch
from
2945a7c to
4e49a54
Compare
oerc0122
force-pushed
the
add-md-geom-parser
branch
2 times, most recently
from
7af8bf2 to
04f2946
Compare
ajjackson
left a comment
ajjackson
left a comment
There was a problem hiding this comment.
This seems useful and nicely written!
I think I see a design issue, however, which can be solved with better separation of the "data container" and "generator" aspects of the object.
| """ | ||
|
|
||
| def __init__(self, md_geom_file: Path | str) -> None: | ||
| self._next_frame = 0 |
There was a problem hiding this comment.
| self._next_frame = 0 | |
| self._next_frame: int | None = 0 |
| pass | ||
| self._frame_len = self._handle.lineno - self._start_line - 1 | ||
|
|
||
| self._byte_len = self._handle.tell() - self._start |
There was a problem hiding this comment.
Maybe something like _frame_bytes would be less confusing. byte_len is 8, surely 😎
| self._len = int(len_est) | ||
|
|
||
| @property | ||
| def next_frame(self) -> int: |
There was a problem hiding this comment.
| def next_frame(self) -> int: | |
| def next_frame(self) -> int | None: |
|
|
||
| @property | ||
| def next_frame(self) -> int: | ||
| """Get index of next frame to be read.""" |
There was a problem hiding this comment.
| """Get index of next frame to be read.""" | |
| """Get index of next frame to be read, or None if at file end.""" |
| MDGeomTimestepInfo | ||
| Parsed frame. | ||
| """ | ||
| if -len(self) > frame > len(self): |
There was a problem hiding this comment.
I think this will always fail for positive values of len and frame.
>>> -3 > 1 > 3
False
Bid odd that it only raises a warning. But I assume that is how the tests are passing?
| def __iter__(self) -> Generator[MDGeomTimestepInfo, int, None]: | ||
| """ | ||
| Get generator over all frames in system. | ||
|
|
||
| Jumps permitted through ``send``. | ||
|
|
||
| Yields | ||
| ------ | ||
| MDGeomTimestepInfo | ||
| Frames in file. | ||
| """ | ||
| self._handle.file.seek(self._start) | ||
| self._handle._lineno = self._start_line | ||
| self._next_frame = 0 | ||
| while self._next_frame is not None: | ||
| jump = yield next(self) | ||
| if jump is not None: | ||
| self._go_to_frame(jump) |
There was a problem hiding this comment.
Some "fun" things (in Dwarf Fortress sense) can happen if multiple iterators are opened from the same instance, as each iterator mutates the same state.
There are some strong opinions on the topic at this StackOverflow: https://stackoverflow.com/questions/46941719/how-can-i-have-multiple-iterators-over-a-single-python-iterable-at-the-same-time
I think Python classes are not supposed to be used as both an Iterator and Iterable. Perhaps this class should choose to act like an Iterable collection (with get-item functionality), not providing a __next__ method. Then the __iter__ code (which acts as a Generator) should maintain its own position information, independent of the main object state, and be able to seek to the next position even if the underlying object is not in sync.
oerc0122
force-pushed
the
add-md-geom-parser
branch
from
04f2946 to
ea31d62
Compare
oerc0122
force-pushed
the
add-md-geom-parser
branch
5 times, most recently
from
a78c3a5 to
1358fed
Compare
No description provided.