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

docs for many AtomGroup methods missing #1845

Closed
orbeckst opened this issue Mar 25, 2018 · 15 comments · Fixed by #2427
Closed

docs for many AtomGroup methods missing #1845

orbeckst opened this issue Mar 25, 2018 · 15 comments · Fixed by #2427

Comments

@orbeckst
Copy link
Member

Expected behaviour

radius_of_gyration(), principal_axes() (see #1828 ), center_of_mass() and most other "compute something on an AtomGroup" (that require e.g., mass) should be in the docs for AtomGroup.

Actual behaviour

They are not in the docs anymore (at least since 0.17.0 but probably since #363 / 0.16.0)

Code to reproduce the behaviour

Look at https://www.mdanalysis.org/mdanalysis/documentation_pages/core/groups.html#MDAnalysis.core.groups.AtomGroup

I think the problem is that we only attached certain methods to AtomGroup when specific TopologyAttrs are present. The bare AtomGroup does not contain masses so many of the most used methods are invisible to the user (and a lot of our docs is broken, e.g., in the tutorial).

Currently version of MDAnalysis:

develop, but also published 0.17.0

@orbeckst
Copy link
Member Author

I don't know how to fix this unless we explicitly write the docs for AtomGroup. @richardjgowers @kain88-de @dotsdl any ideas (eg from datreant) how to properly document when you're doing this magic patching of groups?

From a user's point of view it is absolutely not comprehensible why certain commonly used methods are not documented at all. And it makes it hard to answer user questions because I cannot just link to the docs but have to say "there's this method that just happens to come into existence when you have masses but it's kind of a secret so you need to know about it...".

@richardjgowers
Copy link
Member

richardjgowers commented Mar 25, 2018 via email

@orbeckst
Copy link
Member Author

@richardjgowers What needs to be done to get these docs back? If it's rst hacking then if you show one or two examples then I can do some more. A todo list would also be helpful... or perhaps we can generate the list/rst automatically?

@orbeckst orbeckst added this to the 0.18.x milestone May 1, 2018
@orbeckst
Copy link
Member Author

orbeckst commented Jul 5, 2018

Ping @richardjgowers – if you give me some starters I can chip away on this one. I really want proper docs for 0.19.0!

@orbeckst orbeckst mentioned this issue Jul 5, 2018
9 tasks
@richardjgowers
Copy link
Member

@orbeckst I'm not sure the easiest way to solve this one. The fancy "attaching methods on the fly with attributes" doesn't really lend itself well to how we automatically generate docs. The best solution would be to write the docs manually, (or at least list the methods in order), this would have the benefit that we could control the order & presentation of them

## Position related methods

Blah blah blah, these all take common kwargs like `pbc`, this keyword always does xyz

.. automethod:: `MDAnalysis.core.groups.center`
.. automethod:: `MDAnalysis.core.groups.center_of_geometry`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.center_of_mass`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.radius_of_gyration`

@orbeckst
Copy link
Member Author

orbeckst commented Jul 9, 2018

Ok, then we manually write the docs and add a comment to the code that says that you have to add the appropriate doc entry there and there.

@orbeckst
Copy link
Member Author

orbeckst commented Jul 9, 2018

... or we write a small python script that writes the list for us :-).

u = Universe("full_featured_topology.tpr")
methods = dir(u.atoms)
for method in methods:
  if str(method).startswith("_"):
    continue
  print(".. automethod:: {}".format(str(method)))

... something along those lines

@jbarnoud
Copy link
Contributor

Mouahahahahaha!

import collections
import MDAnalysis as mda

targets = collections.defaultdict(lambda : collections.defaultdict(list))

for attribute_key, attribute in mda.core.topologyattrs._TOPOLOGY_ATTRS.items():
    for target, methods in attribute.transplants.items():
        for method in methods:
            #print(method)
            function = method[1]
            try:
                # We my be dealing with a property; then we need to get the
                # actual method out of it.
                function = function.fget
            except AttributeError:
                # Well, it was not a property
                pass
            text = '.. automethod:: `{}.{}`'.format(function.__module__, function.__qualname__)
            targets[target][attribute_key].append(text)

for target_key, target_dict in targets.items():
    try:
        target_name = target_key.__name__
    except AttributeError:
        # For some reason, some target are not classes but str
        target_name = target_key
    print()
    print(target_name)
    print('=' * len(target_name))
    
    for attribute_key, method_list in target_dict.items():
        print()
        print('Requires ' + attribute_key)
        print('-' * (len('Requires ') + len(attribute_key)))
        print()
        
        for method_text in method_list:
            print(method_text)

Prints:

Residue
=======

Requires names
--------------

.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames._get_named_atom`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.phi_selection`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.psi_selection`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.omega_selection`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.chi1_selection`

Requires name
-------------

.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames._get_named_atom`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.phi_selection`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.psi_selection`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.omega_selection`
.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames.chi1_selection`

AtomGroup
=========

Requires names
--------------

.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames._get_named_atom`

Requires name
-------------

.. automethod:: `MDAnalysis.core.topologyattrs.Atomnames._get_named_atom`

Requires bonds
--------------

.. automethod:: `MDAnalysis.core.topologyattrs.Bonds.fragments`

GroupBase
=========

Requires masses
---------------

.. automethod:: `MDAnalysis.core.topologyattrs.Masses.center_of_mass`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.total_mass`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.moment_of_inertia`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.radius_of_gyration`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.shape_parameter`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.asphericity`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.principal_axes`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.align_principal_axis`

Requires mass
-------------

.. automethod:: `MDAnalysis.core.topologyattrs.Masses.center_of_mass`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.total_mass`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.moment_of_inertia`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.radius_of_gyration`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.shape_parameter`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.asphericity`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.principal_axes`
.. automethod:: `MDAnalysis.core.topologyattrs.Masses.align_principal_axis`

Requires charges
----------------

.. automethod:: `MDAnalysis.core.topologyattrs.Charges.total_charge`

Requires charge
---------------

.. automethod:: `MDAnalysis.core.topologyattrs.Charges.total_charge`

ResidueGroup
============

Requires resnames
-----------------

.. automethod:: `MDAnalysis.core.topologyattrs.Resnames.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Resnames._get_named_residue`
.. automethod:: `MDAnalysis.core.topologyattrs.Resnames.sequence`

Requires resname
----------------

.. automethod:: `MDAnalysis.core.topologyattrs.Resnames.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Resnames._get_named_residue`
.. automethod:: `MDAnalysis.core.topologyattrs.Resnames.sequence`

SegmentGroup
============

Requires segids
---------------

.. automethod:: `MDAnalysis.core.topologyattrs.Segids.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Segids._get_named_segment`

Requires segid
--------------

.. automethod:: `MDAnalysis.core.topologyattrs.Segids.getattr__`
.. automethod:: `MDAnalysis.core.topologyattrs.Segids._get_named_segment`

Atom
====

Requires bonds
--------------

.. automethod:: `MDAnalysis.core.topologyattrs.Bonds.bonded_atoms`
.. automethod:: `MDAnalysis.core.topologyattrs.Bonds.fragment`

Universe
========

Requires models
---------------

.. automethod:: `MDAnalysis.topology.MMTFParser.Models.models`

Requires model
--------------

.. automethod:: `MDAnalysis.topology.MMTFParser.Models.models`

Now, is there a way to have sphinx inject it directly in the right docstrings?

@richardjgowers
Copy link
Member

@jbarnoud that looks like it'd be good enough as a rst stub, then sphinx will do its magic when we build docs

@orbeckst
Copy link
Member Author

Can we re-label these methods so that it looks as if they came from AtomGroup?

.. automethod:: `AtomGroup.center_of_mass <MDAnalysis.core.topologyattrs.Masses.center_of_mass>`

should work.

We will still have to link to the transplant when referring to the method as in

You can compute the center of mass with the :meth:`~MDAnalysis.core.topologyattrs.Masses.center_of_mass` method.

but this is already much better than before.

There might be a way to write our own sphinx plugin to somehow change things later but for right now I'd be happy with going with @jbarnoud 's script. Can you add it to ./maintainer and just write out a file ''doc/sphinx/source/documentation_pages/core/atomgroup_methods.rst'' into the sphinx doc tree, which we can then be included from groups.rst

.. include::  atomgroup_methods.rst

(Search through http://docutils.sourceforge.net/docs/ref/rst/directives.txt because the official reST html page on directives is broken.)

@orbeckst
Copy link
Member Author

orbeckst commented Jul 10, 2018

If we want to include the generated file into the docs for AtomGroup then they may not contain section header markup. Instead, you can use

.. rubric:: Requires masses

and it would also be better to have separate files such as

  • atomgroup_methods.rst
  • atom_methods.rst
  • residuegroup_methods.rst
  • ...

You can also add a comment at the top

.. generated with script maintainer/awesome_method_extractor.py on YYYY-MM-DD
.. commit: deadbeef

EDIT: For getting the current commit:

import subprocess
rev = subprocess.getoutput("git rev-parse HEAD")

@orbeckst
Copy link
Member Author

We are also missing from the containers

  • resids
  • resnums
  • segids (or whatever they are called...)
  • and what did we have for atoms? index? Or did this one go – but then what does the "index" selection look for?

@jbarnoud
Copy link
Contributor

@orbeckst I'll look at this as soon as I have time. I like the .. include:: method. I'll see if I can have sphinx run a script or a function to generate the stub when it builds the doc.

@richardjgowers Do you know if it is possible to distinguish singulars and plurals from mda.core.topologyattrs._TOPOLOGY_ATTRS? Right now I have everything double.

@richardjgowers
Copy link
Member

@jbarnoud iterate over a set of the values of mda._TOPOLOGY_ATTRS then grab the attrname off each TA

# replaces your first loop
for attribute in set(mda._TOPOLOGY_ATTRS.values()):
    attribute_key = attribute.attrname

@jbarnoud
Copy link
Contributor

jbarnoud commented Jul 13, 2018 via email

@orbeckst orbeckst modified the milestones: 0.19.0, 0.19.x Oct 9, 2018
jbarnoud added a commit that referenced this issue Jan 12, 2019
The `transplant_stub.py` script introspects the groups and topology
attributes to write files in `documentation_pages/core` that contain the
documentation for the transplanted methods.

For the stubs to be picked up by sphinx, the docstring of the class to
document must contain

    .. include:: XXX.txt

where "XXX" is the name of the class.

A stub contains a table of the methods, their short descriptions, and
what topology attribute they require.

Fixes #1845
jbarnoud added a commit that referenced this issue Jan 12, 2019
The `transplant_stub.py` script introspects the groups and topology
attributes to write files in `documentation_pages/core` that contain the
documentation for the transplanted methods.

For the stubs to be picked up by sphinx, the docstring of the class to
document must contain

    .. include:: XXX.txt

where "XXX" is the name of the class.

A stub contains a table of the methods, their short descriptions, and
what topology attribute they require.

Fixes #1845
@richardjgowers richardjgowers removed this from the 0.20.x milestone Mar 6, 2020
lilyminium pushed a commit that referenced this issue Oct 22, 2020
- Fixes #1845 
- Adds dummy method with documentation of original method
- Dummy method gets replaced by actual method on instantiation
orbeckst pushed a commit that referenced this issue Oct 23, 2020
- Fixes #1845
- Adds dummy method with documentation of original method
- Dummy method gets replaced by actual method on instantiation

(cherry picked from commit 799100b)
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this issue Mar 30, 2021
- Fixes MDAnalysis#1845 
- Adds dummy method with documentation of original method
- Dummy method gets replaced by actual method on instantiation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment