Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Outdated documentation for AMBER TRJReader #2398

Closed
lilyminium opened this issue Nov 7, 2019 · 4 comments · Fixed by #2595
Closed

Outdated documentation for AMBER TRJReader #2398

lilyminium opened this issue Nov 7, 2019 · 4 comments · Fixed by #2595

Comments

@lilyminium
Copy link
Member

lilyminium commented Nov 7, 2019

Expected behavior

.. rubric:: Limitations
* Periodic boxes are only stored as box lengths A, B, C in an AMBER
trajectory; the reader always assumes that these are orthorhombic
boxes.
* The trajectory does not contain time information so we simply set
the time step to 1 ps (or the user could provide it as kwarg *dt*)
* **No direct access of frames is implemented, only iteration through
the trajectory.**

Functionality is currently limited to simple iteration over the
trajectory.

I cannot directly access frames of the trajectory by indexing.

Actual behavior

I can directly access frames of the trajectory by indexing.

Code to reproduce the behavior

Show us how to reproduce the failiure. If you can, use trajectory files from the test data.

In [28]: import MDAnalysis as mda

In [29]: from MDAnalysis.tests.datafiles import PRM, TRJ

In [30]: trj = mda.Universe(PRM, TRJ)
/Users/lily/pydev/mdanalysis/package/MDAnalysis/topology/TOPParser.py:270: UserWarning: ATOMIC_NUMBER record not found, guessing atom elements based on their atom types
  warnings.warn(msg)

In [34]: trj.trajectory[2]
Out[34]: < Timestep 2 with unit cell dimensions [0. 0. 0. 0. 0. 0.] >

In [35]: t2 = trj.atoms.positions

In [36]: trj.trajectory[5]
Out[36]: < Timestep 5 with unit cell dimensions [0. 0. 0. 0. 0. 0.] >

In [37]: t5 = trj.atoms.positions

In [41]: np.array_equal(t2, t5)
Out[41]: False

Currently version of MDAnalysis

  • Which version are you using? (run python -c "import MDAnalysis as mda; print(mda.__version__)") 0.20.1
  • Which version of Python (python -V)? 3.7.3
  • Which operating system? MacOS

Just checking that I'm understanding this limitation correctly and can leave it out of the user guide.

@orbeckst
Copy link
Member

orbeckst commented Nov 8, 2019

The limitations are outdated. The reader creates offsets for random access.

(I think nowadays all our readers support some form of random access (since PR #1086, see #314 for discussion).)

@orbeckst orbeckst added the defect label Nov 8, 2019
@richardjgowers
Copy link
Member

Yeah this looks like docs I forgot to update in the ascii offset push.

@IAlibay
Copy link
Member

IAlibay commented Feb 29, 2020

@lilyminium My understanding from this is that all that's needed here is just to clean up the docstring (and maybe add a test)?

I'm planning to do some work on the AMBER readers. Unless you were planning on picking this one up, I can make the changes whilst I'm dealing with that file.

@lilyminium
Copy link
Member Author

@IAlibay Yes, I think it's just the documentation here. Please go ahead!

IAlibay added a commit to IAlibay/mdanalysis that referenced this issue Mar 8, 2020
richardjgowers pushed a commit that referenced this issue Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants