Common AnalysisBase framework for storing results

[PicoCentauri]

I'm working together with @joaomcteixeira to implement a command line interface for MDAnalysis. Our implementation is mostly finished 🥳. Currently, we are preparing the repo for moving to the mda organization and make it ready for CI, PYPI, Conda, etc.

However, one important part is still missing. For saving the data a common structure for the results of each analysis class would be helpful. We already started a discussion but since some structural changes to the Analysis Framework and most likely to all analysis classes are necessary I move the discussion.

@orbeckst suggested three possibilities and I will use the rdf class as an example for the API. The current implementation for running and accessing the results looks like

rdf = InterRDF(ag1, ag2).run()
plt.plot(rdf.bins, rdf.rdf)

1. Reserved attribute names

For example, self.results could always be a data structure containing computed data. This data structure could either be
a dictionary

rdf = InterRDF(ag1, ag2).run()
plt.plot(rdf.results["bins"], rdf.results["rdf"])

or attributes

rdf = InterRDF(ag1, ag2).run()
plt.plot(rdf.results.bins, rdf.results.rdf)

2. Trailing underscore attributes (like sklearn-style)

In scikit-learn, computed variables get an underscore affixed (e.g. .results_).

rdf = InterRDF(ag1, ag2).run()
plt.plot(rdf.bins_, rdf.rdf_)

3. Flexible annotation

A dictionary containing a list of attributes for computed data. This dictionary could look like

self._annotation = {'bins': 'OUTPUT', 'rdf': 'OUTPUT',}

Here, OUTPUT could mark anything that is always computed after run() and should be saved. Possible other tags such as "OPTIONAL" for something that might be computed by an auxiliary method. With this approach only a few changes to the classes themselves are necessary. Their python api stays untouched as shown above.

---

My favorite is the first approach using attributes. It combines all results into a structure common for all classes. For me as a user this also handy. I can quickly access all results without always checking the documentation for the correct name.

Regardless of the method we choose, the MDACLI will find 'results' attributes and save them. If a numpy array, we could use np.save() to CSV, if it's a pd.DataFrame, we use to_csv()...

If you have better ideas we are of course open for discussion.

[joaomcteixeira]

Great post @PicoCentauri ,

Thanks for summarizing it so well.

For reference, the issue that seeded this project is #2377

I like the .results attribute. results can be a dict or even a dedicated class that can provide a general API to save the results. Feels natural also to iterate over its items().

[PicoCentauri]

Is there any opinion on the draft? Maybe from the @MDAnalysis/coredevs.

If we decide for a strategy we can push the CLI closer to a first working version. Thanks again for looking at this issue.

[orbeckst]

Did you survey the existing classes? How many do use results? More importantly, how many use results in a way that's not compatible with the above suggestion 1? For example, for RDF results should be a dict storing bins and rdf values but actually store the rdf (I think that's not true for RDF but for e.g., RMSD or RMSF, where the independent variable is something like times or resids). In this case, user code will break when you change the meaning of an existing attribute.

This might be a moot point if you decide that you are directly going for MDAnalysis 2.0.0 where we can make breaking changes more easily although even then we should limit breaking user code. If you will break user code you must put a deprecation note into 1.1.0 (see #3159 )!

Therefore, it would be easier to use a different attribute name or to use the annotation approach 3. (I really dislike the underscore approach, which also does not give you a nice way to collect the attributes... I don't consider introspecting and pattern matching attribute names particularly "nice"... but that's my very personal opinion).

[lilyminium]

I also like the results attribute. results could contain all the output of run(), adding to the class rather than replacing an existing attribute. This would be especially relevant for classes like PCA, where both the eigenvectors and eigenvalues are important.

iirc at least 2 classes use results -- I think encore may be one. None use output as an attribute, I believe.

[PicoCentauri]

Thanks for your feedback. I did a little investigation of the existing analysis classes. An existing results attribute is used by three classes (GNMAnalysis, LinearDensity, PersistenceLength) as well by the AnalysisFromFunction base class.

In brief, we can use a results attribute with acceptable(?) breaking changes in the GNMAnalysis and the LinearDensity class.

Detailed Analysis

GNMAnalysis

results are stored as a list

mdanalysis/package/MDAnalysis/analysis/gnm.py

Line 271 in 6282385

outputobject.append((time, w[list_map[1]], v[list_map[1]]))

LinearDensity

creates a nested dictionary

mdanalysis/package/MDAnalysis/analysis/lineardensity.py

Line 100 in 6282385

self.results = {'x': {'dim': 0}, 'y': {'dim': 1}, 'z': {'dim': 2}}

where for each dimension several values are stored

mdanalysis/package/MDAnalysis/analysis/lineardensity.py

Line 112 in 6282385

self.keys = ['pos', 'pos_std', 'char', 'char_std']

PersistenceLength

Creates a 1D array

mdanalysis/package/MDAnalysis/analysis/polymer.py

Line 202 in 6282385

self._results = np.zeros(chainlength - 1, dtype=np.float32)

AnalysisFromFunction

Stores the return values of the function inside a list of the results attribute

mdanalysis/package/MDAnalysis/analysis/base.py

Line 267 in 6282385

self.results.append(self.function(*self.args, **self.kwargs))

Besides these ones, I didn't found any class that is using results as an attribute. To use results some (breaking) changes are necessary in the GNMAnalysis (list to dict with a key for each list element) and in LinearDensity (nested dict to flat dict with pos -> pos_x for example). The PersistenceLength works directly and the AnalysisFromFunction is not affected at all.

We could also use output as a key attribute as @lilyminium suggested. I didn't found any class that is using output. However, attributes like output and results could confuse users and lead to duplicated code. I would stick with results if somehow possible.

[joaomcteixeira]

Hi all,
Great analysis @PicoCentauri

I believe there won't be any incompatibility with results. For GNMAnalysis and LinearDensity we can create a results @property.getter that converts the list to dict (maybe using dict.fromkeys) or flats the nested dict.

In conclusion, for those classes which results are not a dictionary, a @property method can be used as a parser to convert the native result structures to a dict. That could be also a requirement when writing future analysis classes.

What do you think?

[orbeckst]

Thanks for the analysis, @PicoCentauri . This looks as if results is viable.

Then the next steps are

• What incompatible changes do you need to make to existing code?
  • Any incompatible changes should be deprecated (admittedly, @IAlibay declared a freeze on 1.1.0 so you need to check with him).
  • User Guide examples would need updating
• Define what results is allowed to contain.

If you can make it work without breaking changes (I didn't quite understand where @joaomcteixeira 's property getters would be implemented – in mdacli? ) then you'll have a much easier ride.

[orbeckst]

Btw, this discussion is also relevant for the idea that's been floated on Discord to spin out analysis into "mdkits" that can be maintained separately. Ultimately, each analysis would still appear as if it lived under MDAnalysis.analysis so nothing would immediately change for mdacli.

[IAlibay]

Any incompatible changes should be deprecated (admittedly, @IAlibay declared a freeze on 1.1.0 so you need to check with him).

I'd very much appreciate it if we didn't have any new features. That being said we're unlikely to be moving towards a release until after the end of the month, plus there's a few things that need doing. If it's a simple warning that can go through quick review I think we can live with that.