-
Notifications
You must be signed in to change notification settings - Fork 647
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
More user-friendly docs (Addressing Issue #1175) #1190
Conversation
So Issue #1175 seemed to get warmly received, so I've started an implementation of it. I tried to come up with topics that covers everything to a moderate level, if anyone can think of more topics or a better way to order topics, then this is the PR for that! |
That's a good structure. Now we just need some content... Should we essentially merge this as an incomplete PR and then raise Issues/PRs for things that are still incomplete? Or make PRs against this branch? In any case, I think it would be good to have sub-issues to encourage splitting the load. |
I would advocate against merging the PR as it, even to be filled latter on. This would break the develop branch, which we try to avoid as much as possible. |
We can PR onto this branch, then merge this when everything is done I guess |
:: | ||
|
||
for segment in u.segments: | ||
segment.atoms.guess_bonds() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identation seems wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it's correct on that line. 4 spaces it aligns as it should.
@@ -0,0 +1,7 @@ | |||
.. -*- coding: utf-8 -*- | |||
|
|||
.. All the advanced options in Reader usage and creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also MemoryReader
>>> vel = ag.velocities | ||
NoDataError: Timestep does not contain velocities | ||
|
||
These properties return a **copy** of the data for the AtomGroup at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do really return a copy of the arrays? That seems wasteful and slow in terms of system memory. I thought it was a reference that is returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use fancy slicing on ts._pos
so yes, it will be a copy.
It cannot be a reference: it is a non contiguous slice of the overall array.
…On 03/28/2017 09:58 AM, Max Linke wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In
package/doc/sphinx/source/documentation_pages/coordinate_manipulation.rst
<#1190 (comment)>:
> +properties will always be available from the AtomGroup, however if the loaded
+trajectory does not have this data then a ``NoDataError`` will be raised.
+
+::
+
+ >>> import MDAnalysis as mda
+ >>> from MDAnalysisTests.datafiles import PSF, DCD
+ >>> ag = u.select_atoms('name CA')
+ >>> ag.positions
+ array([[ 11.66462231, 8.39347267, -8.98323059],
+ [ 11.41483879, 5.43442154, -6.51348448],
+ ...
+ >>> vel = ag.velocities
+ NoDataError: Timestep does not contain velocities
+
+These properties return a **copy** of the data for the AtomGroup at the
Do really return a copy of the arrays? That seems wasteful and slow in
terms of system memory. I thought it was a reference that is returned.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1190 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUWupVGZ0PQdnNnDPUsSU2d0MGEn9cOks5rqL29gaJpZM4LvMeQ>.
|
Yeah a numpy fancy index is always a copy, afaik we can't return a
reference/view
On Tue, 28 Mar 2017, 9:00 a.m. Jonathan Barnoud, <notifications@github.com>
wrote:
… It cannot be a reference: it is a non contiguous slice of the overall
array.
On 03/28/2017 09:58 AM, Max Linke wrote:
>
> ***@***.**** commented on this pull request.
>
> ------------------------------------------------------------------------
>
> In
>
package/doc/sphinx/source/documentation_pages/coordinate_manipulation.rst
> <
#1190 (comment)>:
>
> > +properties will always be available from the AtomGroup, however if
the loaded
> +trajectory does not have this data then a ``NoDataError`` will be
raised.
> +
> +::
> +
> + >>> import MDAnalysis as mda
> + >>> from MDAnalysisTests.datafiles import PSF, DCD
> + >>> ag = u.select_atoms('name CA')
> + >>> ag.positions
> + array([[ 11.66462231, 8.39347267, -8.98323059],
> + [ 11.41483879, 5.43442154, -6.51348448],
> + ...
> + >>> vel = ag.velocities
> + NoDataError: Timestep does not contain velocities
> +
> +These properties return a **copy** of the data for the AtomGroup at the
>
> Do really return a copy of the arrays? That seems wasteful and slow in
> terms of system memory. I thought it was a reference that is returned.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <
#1190 (review)>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/ABUWupVGZ0PQdnNnDPUsSU2d0MGEn9cOks5rqL29gaJpZM4LvMeQ
>.
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1190 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AI0jB9Bk0CI3FoKnKuK5qa_nNo48PctGks5rqL4ZgaJpZM4LvMeQ>
.
|
@richardjgowers is this PR something we want to get into 0.16.x or should we re-target to 0.17.0? |
@orbeckst ideally it gets merged as soon as its finished. This isn't code breaking/making so I don't think we're bound by version numbers. |
|
||
|
||
API Reference | ||
============= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the sidebar navigation in #1126 I am pretty sure that we need a single toctree. I would suggest to keep the top-toctree pretty light, essentially
- Overview
- User guide
- API Reference
- References
where User guide is what you called "MDAnalysis Basics"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rebase after #1126 has been merged and see if the docs are acceptable. Then we can add it to 0.16.1; otherwise we keep working on it and put it into the next release.
Docs split into User guide, then API reference: outlining the general structure of the docs.
- bonds angles and torsions section of docs - Added rst docs for bonds angles torsions - Updated topologyobjects docstrings - Update advanced_reader.rst - Update advanced_universe.rst
I am currently rebasing to develop and cleaning up the history. I will force-push to this branch. |
85ee177
to
c5d6d6c
Compare
I moved it to 0.17.0 – I don't think that anyone has currently got the bandwidth to make this happen for 0.16.2. |
I moved it to 1.0. Obviously, any improvements to the docs are more than welcome at any time, but this should not hold up a 0.17. |
Will the completion of the UserGuide by @lilyminium make this PR obsolete? I think we still want to clean up the user docs and make them more API centric and have the UserGuide as the explanatory document – opinions? |
I think this is obsolete thanks to @lilyminium |
Docs split into User guide, then API reference
Fixes #1175
Changes made in this Pull Request:
PR Checklist