ChainReader now subclasses BaseReader #3906
Conversation
| #Overrides :meth:`~MDAnalysis.coordinates.base.ProtoReader.add_transformations` | ||
| #to avoid unintended behaviour where the coordinates of each frame are transformed | ||
| #multiple times when iterating over the trajectory. | ||
| #In this method, the trajectory is modified all at once and once only. |
There was a problem hiding this comment.
This comment suggests that there was a reason for applying transformations to individual trajectories instead of to the ChainReader itself. Did you look into this reason more closely?
There was a problem hiding this comment.
test coverage for this sort of behaviour seems good, see: https://github.com/MDAnalysis/mdanalysis/blob/develop/testsuite/MDAnalysisTests/coordinates/test_chainreader.py#L138
richardjgowers
force-pushed
the
chainreader_subclasses_ReaderBase
branch
from
cfd04ad
to
23ddffe
Compare
|
Is it realistic to have this in 2.5 or can we move it to 2.6? |
|
Moving to a 2.6.0 target |
richardjgowers
force-pushed
the
chainreader_subclasses_ReaderBase
branch
from
23ddffe
to
66937df
Compare
Linter Bot Results:Hi @richardjgowers! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #3906 +/- ##
===========================================
- Coverage 93.60% 93.60% -0.01%
===========================================
Files 192 192
Lines 25146 25141 -5
Branches 4058 4058
===========================================
- Hits 23539 23532 -7
- Misses 1089 1092 +3
+ Partials 518 517 -1
☔ View full report in Codecov by Sentry. |
richardjgowers
changed the title
| @@ -159,7 +158,7 @@ def check_allowed_filetypes(readers, allowed): | |||
| "supported for formats: {}".format(allowed)) | |||
|
|
|||
|
|
|||
| class ChainReader(base.ProtoReader): | |||
| class ChainReader(base.ReaderBase): | |||
There was a problem hiding this comment.
this line change is what's happening in this PR. Previously by subclassing ProtoReader (which is a more distant ancestor) you had to also reimplement how transformations worked.
Instead ChainReader now subclasses ReaderBase (like a "normal" reader) and now only implements _read_next_timestep (like a "normal" Reader). This means that methods __next__, copy, add_transformations are all done by the base class.
tl;dr less code = less bugs
|
I'm not sure what the pylint failure means, it's not a line I've touched so I'm not going to try and understand it |
| @@ -614,7 +613,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |||
| return False # do not suppress exceptions | |||
|
|
|||
|
|
|||
| class _Readermeta(type): | |||
| class _Readermeta(abc.ABCMeta): | |||
There was a problem hiding this comment.
re: pylint failure, do you not need to do a super.init call or something on line 625?
There was a problem hiding this comment.
Honestly code like this is very much in the "if it ain't broke don't fix it" category for me
There was a problem hiding this comment.
So remove the linter?
There was a problem hiding this comment.
no there's a # comment you can use - it's in the pylint docs iirc
Fixes #4008
Fixes #3657
Changes made in this Pull Request:
ChainReader currently subclasses
ProtoReaderrather thanReaderBase, then attempts to implement many things regarding auxiliaries and transformations itself. I think it's getting many subtle things wrong with aux and transformations.This PR makes
ChainReaderjust another subclass ofReaderBase, which ultimately simplifies it.PR Checklist