Skip to content

Generate doc tables for transplanted methods, continued#2418

Closed
lilyminium wants to merge 12 commits intoMDAnalysis:developfrom
lilyminium:stub-cont
Closed

Generate doc tables for transplanted methods, continued#2418
lilyminium wants to merge 12 commits intoMDAnalysis:developfrom
lilyminium:stub-cont

Conversation

@lilyminium
Copy link
Member

@lilyminium lilyminium commented Dec 20, 2019

Fixes #1845

Continues #2176
Changes made in this Pull Request:

  • Writes a sorted list of methods and properties without arguments like group or self.

I haven't addressed the duplicate citation issue. Links just go to the original MDAnalysis.core.topologyattrs.Attrname.Method docstring citation.

To be honest, IMO the best solution would be to just move the methods to the core classes and fail gracefully when the required topology attribute isn't there. This approach has many downsides, including:

  • I sorted the properties/methods alphabetically, but all of them nevertheless appear before the *Group's actual properties/methods
  • You must manually add .. include:: XXX_methods_table.txt and .. include:: XXX_methods_docs.txt to each docstring every time you add a new transplant target
  • The Universe transplanted methods are now in a docstring that's largely about its __init__ method and will show up in help(Universe)
  • citations confusingly link to an identical entry on a different page
  • People looking at the code now have a mismatch between code and documentation
  • what happens when you have a method that requires >1 topology attribute?

PR Checklist

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

jbarnoud and others added 9 commits January 12, 2019 21:57
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 MDAnalysis#1845
The syntax is not supported in python 3.5
Locally, sphinx complains about citation duplicates. Though, travis
complains about missing citations refering about the same citation.
@lilyminium
Copy link
Member Author

Looks like the screenshot below or have a look at these built docs. I felt a little blurb was needed to explain the sudden table.

Screenshot 2019-12-20 at 5 14 55 PM

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #2418 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2418   +/-   ##
========================================
  Coverage    90.12%   90.12%           
========================================
  Files          177      177           
  Lines        22511    22511           
  Branches      2913     2913           
========================================
  Hits         20288    20288           
  Misses        1620     1620           
  Partials       603      603
Impacted Files Coverage Δ
package/MDAnalysis/core/universe.py 95.23% <ø> (ø) ⬆️
package/MDAnalysis/core/groups.py 98.22% <ø> (ø) ⬆️

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 4adbd6c...6ccd4a5. Read the comment docs.

@richardjgowers
Copy link
Member

I’m ok with moving methods into the base class. I think the whole transplanted methods thing went a little too far.

@jbarnoud
Copy link
Contributor

To add to @lilyminium argument, having the transplanted method also means that the methods are less discoverable, that they are clearly not IDE friendly, and that error messages are unhelpful when switching topology format.

@jbarnoud
Copy link
Contributor

I’m ok with moving methods into the base class. I think the whole transplanted methods thing went a little too far.

I gave a go at the move in #2425. I am not done with it, but it feels already like a massive regression. The reasons @richardjgowers gave to justify the transplanted methods stand and become more obvious when undoing the change.

From #601:

Reasons for this approach

  • Prevents the base AtomGroup class from becoming bloated with every imaginable method
  • All code related to a given Attribute lives in the same module
  • Can include extra Attributes & methods without installing MDA from source

@orbeckst
Copy link
Member

This PR has been superseded by PR #2427 and #3007 so I am closing it.

@orbeckst orbeckst closed this Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs for many AtomGroup methods missing

4 participants