RFC: Add results class for storing analysis results

[PicoCentauri]

Fixes #3115

Changes made in this Pull Request:
This PR adds a _Results class to the AnalysisBase for similarly storing results across all analysis modules. One could think about using Python's "new" dataclass decorator when Py3.6 is dropped.

I would like to know what you think about the approach and I'm open to discussing improvements or other suggestions.
The docs definitely need some polishing.

PR Checklist

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

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-05 07:29:03 UTC

[codecov]

Codecov Report

Merging #3233 (85940f9) into develop (79183bf) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #3233      +/-   ##
===========================================
+ Coverage    92.87%   92.88%   +0.01%     
===========================================
  Files          170      170              
  Lines        22870    22905      +35     
  Branches      3243     3249       +6     
===========================================
+ Hits         21240    21275      +35     
  Misses        1582     1582              
  Partials        48       48              

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/__init__.py	100.00% <ø> (ø)
package/MDAnalysis/analysis/base.py	96.63% <100.00%> (+1.18%)	⬆️
package/MDAnalysis/analysis/gnm.py	99.20% <100.00%> (+0.02%)	⬆️
package/MDAnalysis/analysis/lineardensity.py	84.41% <100.00%> (+0.20%)	⬆️
package/MDAnalysis/analysis/polymer.py	100.00% <100.00%> (ø)

---

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 79183bf...85940f9. Read the comment docs.

[IAlibay]

Just going to ping all the @MDAnalysis/coredevs this is central to the 2.0 release so the more eyes the better.

[orbeckst]

• I think the _Results class could be made more dict like, which would also help with integrating it into the existing classes.
• I am not clear how AnalysisFromFunction will use the new results class.

[PicoCentauri]

I'm wondering and don't see a reason why GNMAnalysis is not inheriting from AnalysisBase. I could adjust this here or create a different PR to fix this.

[orbeckst]

I'm wondering and don't see a reason why GNMAnalysis is not inheriting from AnalysisBase. I could adjust this here or create a different PR to fix this.

History... very old code.

Would be awesome to have it AnalysisBase-derived but better in a different PR – small, self-contained PRs are more easily reviewed and merged.

[orbeckst]

If you raise an issue then you can attach the PR to the issue for making GNM analysis canonical.

[orbeckst]

With GNM now AnalysisBase-based #3244, could you please update the PR, @PicoCentauri ?

[orbeckst]

I am pretty much ready to approve this PR and the use of the Results class for holding results.

@MDAnalysis/coredevs if you see issues with this approach, now is the time to make a blocking review.

[orbeckst]

I am generally happy with the approach.

The two major things I see

• document what API should contain (or that it contain anything)
• prevent Results to have keys that collide with dict methods/attributes

Otherwise I have various minor doc things – please see comments.

[orbeckst]

Looking very good, just minor doc/style comments.

[orbeckst]

I didn't have time to see why the tests fail but obviously they should pass, too.

[lilyminium]

I saw you updated while I was reviewing so not all comments may be applicable @PicoCentauri. I've left comments on the docs :)

[PicoCentauri]

Thank for your suggestion @orbeckst. I implemented the doc changes. While removing the available analysis section I merged the docstring of the analysis' __init__.py into `analysis_modules.rst. For me this seemed more reasonable as an extra documentation page with only one paragraph.

[orbeckst]

@lilyminium if you have a few minutes to spare to do a final look at this PR then please do. Otherwise I am happy to merge it tonight (so that we can add Results to other analysis classes – see #3115 (comment) ).

@IAlibay , you said on discord you found some duplicate authorship/names — I didn't understand what that was about. Anything that needs to be fixed here?

[IAlibay]

@IAlibay , you said on discord you found some duplicate authorship/names — I didn't understand what that was about. Anything that needs to be fixed here?

O yeah, nothing major, there's just two PicoCentauri entries in the CHANGELOG for 2.0. We can just remove it when we update a few of the other AnalysisBase things to use Results (i.e. there's no point to triggering CI again just for that).

[PicoCentauri]

O yeah, nothing major, there's just two PicoCentauri entries in the CHANGELOG for 2.0. We can just remove it when we update a few of the other AnalysisBase things to use Results (i.e. there's no point to triggering CI again just for that).

Sorry, that was probably me who messed this up.

Comments were addressed as far as I can see and @lilyminium said I should dismiss her review if necessary as she is busy.

[orbeckst]

@PicoCentauri and @joaomcteixeira – the new AnalysisBase is in 2.0.0 !

As @IAlibay said: let's get at least the most used AnalysisBase classes to also use .results (in a backward compatible way) so that we can teach the new way at the PRACE workshop.

[joaomcteixeira]

Fantastic! All kudos for this implementation to @PicoCentauri. Congratz! 🎉