Cleanup and restructure of AnalysisBase classes docstrings

[PicoCentauri]

Fixes #919 Fixes #3651

Changes made in this Pull Request:

• Deprecate verbose parameter in AnalysisBase.__init__ (follows issue #3651)
• Moved all docstrings from __init__ function to class docstring (follows issue #919)
• Removed doubled docs in EinsteinMSD
• Removed docs for non existing verbose parameter in conformational_distance_matrix
• Cleanup and sorted imports of all touched files

I also restructured the docs of the RMS classes a bit. If you disagree with anything please let me know. I checked the rendered docs and hope I did not oversaw any issues within the html pages. These are many changed lines, but I saw a lot of things while I created the decorator. Thanks for your feedback and your time to review!

PR Checklist

• [x] Tests?
• [x] Docs?
• [x] CHANGELOG updated?
• [x] Issue raised/referenced?

[pep8speaks]

Hello @PicoCentauri! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/analysis/bat.py:

Line 273:80: E501 line too long (85 > 79 characters)

• In the file package/MDAnalysis/analysis/pca.py:

Line 276:80: E501 line too long (81 > 79 characters)

Comment last updated at 2022-04-26 13:47:21 UTC

[codecov[bot]]

Codecov Report

Merging #3644 (d4210a7) into develop (3a03af0) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3644   +/-   ##
========================================
  Coverage    94.29%   94.30%           
========================================
  Files          191      191           
  Lines        24844    24813   -31     
  Branches      3349     3345    -4     
========================================
- Hits         23426    23399   -27     
+ Misses        1370     1366    -4     
  Partials        48       48           

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/lineardensity.py	87.50% <ø> (-0.16%)	:arrow_down:
package/MDAnalysis/analysis/rdf.py	100.00% <ø> (ø)
package/MDAnalysis/analysis/align.py	98.32% <100.00%> (ø)
package/MDAnalysis/analysis/base.py	100.00% <100.00%> (+3.20%)	:arrow_up:
package/MDAnalysis/analysis/bat.py	97.57% <100.00%> (-0.02%)	:arrow_down:
package/MDAnalysis/analysis/contacts.py	100.00% <100.00%> (ø)
package/MDAnalysis/analysis/density.py	100.00% <100.00%> (ø)
package/MDAnalysis/analysis/dielectric.py	100.00% <100.00%> (ø)
package/MDAnalysis/analysis/diffusionmap.py	100.00% <100.00%> (ø)
package/MDAnalysis/analysis/dihedrals.py	100.00% <100.00%> (ø)
... and 11 more

---

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 3a03af0...d4210a7. Read the comment docs.

[lilyminium]

The decorator is interesting. I suppose it makes things more consistent, but it does make writing documentation a bit more annoying (having to check what string is actually replaced)... and you have the same problem where verbose is not in the documentation unless the author remembers to import and use it. @MDAnalysis/coredevs any thoughts? Tbh we could go all the way and write a metaclass that automatically appends base class arguments to the docs, although that sounds really complicated.

All the other docs look good to me, although the import sorting -- did you use isort? If we want to maintain alphabetical imports we could look into a more long-term solution like pre-commit hooks.

[lilyminium]

Actually, do you know if the docstring decorator works well in IDEs and/or help?

[PicoCentauri]

The decorator is interesting. I suppose it makes things more consistent, but it does make writing documentation a bit more annoying (having to check what string is actually replaced)... and you have the same problem where verbose is not in the documentation unless the author remembers to import and use it. https://github.com/orgs/MDAnalysis/teams/coredevs any thoughts? Tbh we could go all the way and write a metaclass that automatically appends base class arguments to the docs, although that sounds really complicated.

Of course, authors have to remember but the decorator is consistent and we could add this to documentation for writing new classes based on the AnalysisBase class. A metaclass would be indeed optimal. But This would include parsing the docstring, which could be a bit painful as I learned from the cli.

All the other docs look good to me, although the import sorting -- did you use isort? If we want to maintain alphabetical imports we could look into a more long-term solution like pre-commit hooks.

I used isort indeed. A pre-commit hook sounds like something very nice. The alphabetical order is a choice (not the only one) but gives us developers a guideline and prevents chaotic and vast import sections.

[PicoCentauri]

Actually, do you know if the docstring decorator works well in IDEs and/or help?

I works in help and with the ? syntax. However at least vscode does not parse it 'correctly'.

[jbarnoud]

While I appreciate the improved consistency, I am not sure I like the decorator as a solution. The AnalysisBase API to be adopted, needs to be as streamlined as possible. The issue I see with the decorator is that, on top of inheriting from the base class and overwriting the expected methods, a user needs to add a class decorator and the template placeholder. These two extra things are MDAnalysis-specific knowledge, which adds to the issue, I think.

What plays in favour of the solution, in my eyes, is that it may be considered for internal use only. Then, the users do not need to care. But it also means our analyses are a bit less useful as templates/examples for the users.

It makes me remember, didn't we discuss deprecating verbose in the __init__ in favour of the one in run?

[IAlibay]

So my 2 cents is that a) 90% of my docstring reading is happening when I'm reading the code itself, b) for the remaining 10%, 9% is happening in VSCode... Now I realise that both those use cases are probably "me specific", so I'm not going to say it's a blocker, but I would strongly support alternative solutions 😅

It makes me remember, didn't we discuss deprecating verbose in the __init__ in favour of the one in run?

Yeah I thought that was the case too, can't find the relevant issue but I swear that was in the plans. Honestly I'd prefer we just get rid of it, verbose is somewhat useless for class construction...

[jbarnoud]

So my 2 cents is that a) 90% of my docstring reading is happening when I'm reading the code itself, b) for the remaining 10%, 9% is happening in VSCode...

That too!

[PicoCentauri]

I just want to get rid off the 20 different phrases of describing a verbose option. I am happy with removing the verbose parameter completely for the __init__ function. I also remember that there was a discussion about it somewhere @IAlibay @jbarnoud 😅

I can deprecate the verbose parameter in the AnalysisBase and remove all verbose parameters from the classes if the @MDAnalysis/coredevs agree?

[jbarnoud]

I can deprecate the verbose parameter in the AnalysisBase and remove all verbose parameters from the classes

I would be up for it.

[IAlibay]

I just want to get rid off the 20 different phrases of describing a verbose option. I am happy with removing the verbose parameter completely for the __init__ function. I also remember that there was a discussion about it somewhere @IAlibay @jbarnoud 😅

I can deprecate the verbose parameter in the AnalysisBase and remove all verbose parameters from the classes if the @MDAnalysis/coredevs agree?

I'd possibly bringing it up in a separate issue (given we all seem to have forgotten where the original issue was) and pinging the #developers channel on discord with the issue. Given how close we are to GSoC / Outreachy deadlines there's always a risk that the current discussion will get lost in the sea of notifications.

[PicoCentauri]

I can create a new one but at least @orbeckst commented in #3431 that we could remove the verbose parameter which and maybe also remove the **kwargs argument...

[jbarnoud]

We cannot remove kwargs here, otherwise third party code will break every time we change the signature.

[jbarnoud]

I mean that for AnalysisBase. The other classes can loose the kwarg.

[richardjgowers]

WRT removing verbose, I think we can have verbose in one place, but I think it'll have to be __init__ else how will we see log stuff that happens during init?

[IAlibay]

I can't think of any analysisbase method where verbosity in init is even a thing - my 2 cent is that verbose is within the realm of tqdm and the like i.e. run only. If you want to be informative on setup then logger.info might be where we want to go...

[PicoCentauri]

I now removed the explicit verbose option from the module initialization of AnalysisBase. This brought a little cascade of issues I had to fix. The main questions are what to do with _filter_baseanalysis_kwargs and is it okay to move the logging in the align classes?

The is no rush with this PR and we can discuss this when there is more time after the deadlines of GSOC. I want to sort and document my ideas before I forget everything again.

[orbeckst]

@PicoCentauri really stupid question: the current PR doesn't contain a verbosity decorator anymore, does it? It just deprecates the verbose kwarg in AnalysisBase.__init__ and takes it from the **kwargs, right?

[orbeckst]

If there's no decorator in there anymore then please update the PR comment and also reference issue #3651.

[PicoCentauri]

@PicoCentauri really stupid question: the current PR doesn't contain a verbosity decorator anymore, does it? It just deprecates the verbose kwarg in AnalysisBase.init and takes it from the **kwargs, right?

Yes! I changed the description accordingly.