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.

Sign up for GitHub

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

timestep #4447

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

timestep #4447

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
70e036f
tim
nataliyah123 Feb 3, 2024
f97dd30
timestep not working
nataliyah123 Feb 17, 2024
1af773c
timestep errors _v1
nataliyah123 Feb 17, 2024
2df693b
minor mistakes
nataliyah123 Feb 17, 2024
18696b5
rounding the values as requested
nataliyah123 Feb 20, 2024
198eaf3
some checks
nataliyah123 Feb 21, 2024
a545ba2
some_changes_v2
nataliyah123 Feb 21, 2024
ac4519b
recompiled timestep.py
nataliyah123 Feb 22, 2024
a5bb78e
Update AUTHORS
nataliyah123 Mar 11, 2024
9cbc432
Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…
nataliyah123 Mar 11, 2024
578218f
added to authors
nataliyah123 Mar 11, 2024
ba8dee3
Merge branch 'timestep' of https://github.com/nataliyah123/mdanalysis…
nataliyah123 Mar 11, 2024
1e5ae0d
added to changelog
nataliyah123 Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package/AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,11 @@ Chronological list of authors
- Lawson Woods
- Johannes Stöckelmaier
- Jenna M. Swarthout Goddard
2024

2024
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2024
2024

Just need to revert this back to the previous formatting.

Copy link
Contributor Author

@nataliyah123 nataliyah123 Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@IAlibay all the rest of the years in the Authors file do not have any space except for 2024 which has two spaces, I have aligned the year 2024 with the rest of the years, if you still want me to revert the changes, I will do that.

Copy link
Member

Choose a reason for hiding this comment

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

image

Sorry @nataliyah123 I don't think I fully understand what you mean. The image above (of the 2022 entry) is how it should look for 2024. My suggested change should fix things to how they should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IAlibay made the changes you suggested in both files. and reverted the changes you asked. I was talking about the spaces as shown in the image below.
image

- Aditya Keshari
- Philipp Stärk
- Sumaira Ahmad

External code
-------------
Expand Down
33 changes: 22 additions & 11 deletions package/MDAnalysis/coordinates/timestep.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,20 +493,31 @@ cdef class Timestep:
vectors lengths followed by their respective angle, or as three
triclinic vectors.

>>> ts.dimensions
array([ 13., 14., 15., 90., 90., 90.], dtype=float32)
>>> ts.triclinic_dimensions
array([[ 13., 0., 0.],
[ 0., 14., 0.],
[ 0., 0., 15.]], dtype=float32)
.. testsetup::

>>> import MDAnalysis as mda
>>> from MDAnalysis.tests.datafiles import TPR, XTC
>>> import numpy as np
>>> u = mda.Universe(TPR, XTC)
>>> ts = u.trajectory[0]
Comment on lines +498 to +502
Copy link
Member

Choose a reason for hiding this comment

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

Based on a similar PR https://github.com/MDAnalysis/mdanalysis/pull/4374/files these lines should be indented to look like

         .. testsetup::
            import MDAnalysis as mda
            from MDAnalysis.tests.datafiles import TPR, XTC
            ...

and I am not 100% sure if >>> should remain.


.. doctest::

>>> print(np.round(ts.dimensions))
Copy link
Member

Choose a reason for hiding this comment

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

indentation?

Copy link
Member

Choose a reason for hiding this comment

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

Is rounding really required? It makes for less clear documentation. I'd like to get rid of it if at all possible so that it does not distract from what we are really interested in, namely ts.dimensions. The documentation aspect is more important than the testing...

[80. 80. 80. 60. 60. 90.]
>>> print(np.round(ts.triclinic_dimensions))
[[80. 0. 0.]
[ 0. 80. 0.]
[40. 40. 57.]]

Setting the attribute also works::


.. doctest::

>>> ts.triclinic_dimensions = [[15, 0, 0], [5, 15, 0], [5, 5, 15]]
>>> ts.dimensions
array([ 15. , 15.81138802, 16.58312416, 67.58049774,
72.45159912, 71.56504822], dtype=float32)

>>> print(np.round(ts.dimensions))
Copy link
Member

Choose a reason for hiding this comment

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

rounding here just gives the wrong output. Find a way to do this without round.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orbeckst I am getting this error

autodoc: failed to import module 'hole2' from module 'MDAnalysis.analysis'; the following exception was raised:
No module named 'mdahole2'
make: *** [Makefile:130: doctest] Error 2

I think mdahole2 uses python 3.11 or less I am using 3.12 so I am unable to install mdahole2. please check the error report I have attached in an earlier message. I will make the requested changes once this is resolved

Copy link
Member

Choose a reason for hiding this comment

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

You might have to install in a Python 3.11 environment for testing. The development version is a work in progress and even though we strive to make it fully usable, this is not always possible until close to a release.

[15. 16. 17. 68. 72. 72.]

See Also
--------
:func:`MDAnalysis.lib.mdamath.triclinic_vectors`
Expand Down
Loading