Skip to content

Chainreader Refactoring#2815

Merged
orbeckst merged 14 commits intoMDAnalysis:developfrom
yuxuanzhuang:chainreader_new
Jul 3, 2020
Merged

Chainreader Refactoring#2815
orbeckst merged 14 commits intoMDAnalysis:developfrom
yuxuanzhuang:chainreader_new

Conversation

@yuxuanzhuang
Copy link
Contributor

@yuxuanzhuang yuxuanzhuang commented Jul 2, 2020 •
edited
Loading

Fixes #2814

Changes made in this Pull Request:

  • new _read_frame and __iter__ are connected.
  • remove _chained_iterator.
  • add __next__ to ChainReader.
  • add an extra attribute current_frame to internally monitor absolute current frame.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@yuxuanzhuang yuxuanzhuang mentioned this pull request Jul 2, 2020
Merged
4 tasks
@codecov
Copy link

codecov bot commented Jul 2, 2020 •
edited
Loading

Codecov Report

Merging #2815 into develop will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2815      +/-   ##
===========================================
- Coverage    92.22%   92.22%   -0.01%     
===========================================
  Files          184      184              
  Lines        24140    24139       -1     
  Branches      3122     3122              
===========================================
- Hits         22262    22261       -1     
  Misses        1813     1813              
  Partials        65       65
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/chain.py 92.27% <100.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61e236d...5b09fbd. Read the comment docs.

orbeckst
orbeckst reviewed Jul 2, 2020
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good – I am waiting for CI and other comments in case I overlooked something.

EDIT: please add entry to CHANGELOG

kain88-de
kain88-de reviewed Jul 2, 2020
View reviewed changes
package/MDAnalysis/coordinates/chain.py Outdated
raise StopIteration()

def next(self):
return self.__next__()
Copy link
Member

@kain88-de kain88-de Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next method is not needed anymore. See pep-3114. I know we are not very good in following this pattern.

Copy link
Member

@orbeckst orbeckst Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we backport this fix to 1.0.1 #2798 , will we need to add next() back for Python 2.7?

Copy link
Contributor Author

@yuxuanzhuang yuxuanzhuang Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next() still functions here from ProtoReader definition.

Copy link
Member

@kain88-de kain88-de Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Than we should leave the next function and remove it once from all readers.

kain88-de
kain88-de reviewed Jul 2, 2020
View reviewed changes
testsuite/MDAnalysisTests/coordinates/test_chainreader.py
def test_next_after_frame_numbering(self, universe):
universe.trajectory[98] # index is 0-based and frames are 0-based
universe.trajectory.next()
assert_equal(universe.trajectory.frame, 99, "wrong frame number")
Copy link
Member

@kain88-de kain88-de Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this failed previously? Wow.

orbeckst
orbeckst approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, assuming tests pass

@orbeckst
Copy link
Member

orbeckst commented Jul 2, 2020 •
edited
Loading

Not really sure why the CI is not running on this PR....

EDIT (5s later): now it does. Sorry for the noise.

@orbeckst orbeckst self-assigned this Jul 3, 2020
@orbeckst orbeckst merged commit e878e5d into MDAnalysis:develop Jul 3, 2020
@orbeckst
Copy link
Member

orbeckst commented Jul 3, 2020

Thanks a lot @yuxuanzhuang !

orbeckst pushed a commit that referenced this pull request Jul 3, 2020
@yuxuanzhuang @orbeckst
ChainReader Refactoring (#2815)
eeb6921 
* Fixes #2814
* changes in ChainReader:
   * new `_read_frame` and `__iter__` are now properly connected
      (previously, they moved independently, which could lead to the ChainReader
      being in an inconsistent state)
   * remove `_chained_iterator`.
   * add `__next__`
   * add an extra attribute `__current_frame` to internally monitor absolute current frame
* add tests
* update CHANGELOG
orbeckst pushed a commit that referenced this pull request Jul 12, 2020
@yuxuanzhuang @orbeckst
ChainReader Refactoring (#2815)
fa6bc1f 
* Fixes #2814
* changes in ChainReader:
   * new `_read_frame` and `__iter__` are now properly connected
      (previously, they moved independently, which could lead to the ChainReader
      being in an inconsistent state)
   * remove `_chained_iterator`.
   * add `__next__`
   * add an extra attribute `__current_frame` to internally monitor absolute current frame
* add tests
* update CHANGELOG
@orbeckst orbeckst mentioned this pull request Aug 16, 2020
Closed
4 tasks
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
@yuxuanzhuang @PicoCentauri
ChainReader Refactoring (MDAnalysis#2815)
0b4163d 
* Fixes MDAnalysis#2814
* changes in ChainReader:
   * new `_read_frame` and `__iter__` are now properly connected
      (previously, they moved independently, which could lead to the ChainReader
      being in an inconsistent state)
   * remove `_chained_iterator`.
   * add `__next__`
   * add an extra attribute `__current_frame` to internally monitor absolute current frame
* add tests
* update CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@orbeckst orbeckst orbeckst approved these changes

@kain88-de kain88-de kain88-de approved these changes

Assignees

@orbeckst orbeckst

Labels

Component-Readers defect

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ChainReader read_frame does not update iteration

4 participants

@yuxuanzhuang @orbeckst @kain88-de @fiona-naughton