Add an `AnalysisCollection` class #4017

PicoCentauri · 2023-01-30T10:15:36Z

The AnalysisCollection follows the discussion of #3569 resetting the timestep object for each analysis by default. If requested the user can set the reset_timestep variable to False allowing altering the timestep object.

Within this PR I moved the run method from AnalysisBase to AnalysisCollection to avoid code a lot of duplication. These changes interface of Github might make this a bit difficult to follow. The core changes the run method only that instead of running the _prepare, _single_frame and _conclude methods only once it is looped over all provided analysis instances.

Changes made in this Pull Request:

Add an AnalysisCollection class to perform multiple analyses on the same trajectory
Use assert_equal in test_base.py

PR Checklist

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

Disclaimer: This PR was written with inspiration of OpenAI's ChatGPT.

codecov · 2023-01-30T10:33:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.17%. Comparing base (347a0c0) to head (e762e6a).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4017      +/-   ##
===========================================
- Coverage    93.59%   93.17%   -0.43%     
===========================================
  Files          168       12     -156     
  Lines        21104     1069   -20035     
  Branches      3919        0    -3919     
===========================================
- Hits         19752      996   -18756     
+ Misses         894       73     -821     
+ Partials       458        0     -458

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

richardjgowers

I like this. Can I be cheeky and inquire what sort of speed up you get running something like 2 separate rdfs in tandem?

richardjgowers · 2023-02-15T18:25:54Z

package/MDAnalysis/analysis/base.py

+                analysis_object.times[i] = ts.time
+                analysis_object._single_frame()
+
+                if reset_timestep:


Ts is never unassigned, so I'm not sure this is necessary here

But, if an instance is changing the ts object we have to restore it from the stored one.

richardjgowers · 2023-02-15T18:27:58Z

package/MDAnalysis/analysis/base.py

+                ts_original = ts.copy()
+
+            for analysis_object in self._analysis_objects:
+                analysis_object._frame_index = i


This is messy but understandable. In hindsight, maybe passing these variables through the single frame method would have been cleaner.

Yes, maybe be, but changing this might break a lot. I can put a comment so that these setters might become clearer.

PicoCentauri · 2023-02-16T15:29:05Z

If I use the code given in the example of class I got a speedup of ~30% compared to running each class individually. Quite good for such a simple change 🙂

orbeckst · 2023-02-24T22:17:43Z

@richardjgowers I made you the responsible adult in the room ;-) — please shepherd the PR to completion

yuxuanzhuang · 2023-02-25T10:30:58Z

Up for discussion, would it be a good idea to inherit abc.Collection class so that one can do e.g.

for analysis in collection:
    print(analysis.results)

Besides, it feels a bit weird to me that AnalysisBase is inherited from AnalysisCollection which is a collection of AnalysisBases...

orbeckst

I do not like turning the core of AnalysisBase inside out and into AnalysisCollection (ie run()).

It's semantically confusing and it also makes it even harder to understand how to write your own analysis.

I think the logic needs to be rethought.

orbeckst · 2023-02-25T18:24:53Z

package/MDAnalysis/analysis/base.py

+        return self
+
+
+class AnalysisBase(AnalysisCollection):


I agree with @yuxuanzhuang : it feels very weird and circular to have AnalysisBase inherited from AnalysisCollection. It just does not make sense semantically.

Can this be changed?

The only thing why I did this to do not copy code of the run method between AnalysisBase and AnalysisCollection. We can remove the inheritance by explicitly leaving the run method in AnalysisBase untouched. This would be fine for me!

PicoCentauri · 2023-02-25T18:56:03Z

We could wither duplicate the logic, which I don't like. Or, create en external class that both the Base and the Collection use.

yuxuanzhuang · 2023-02-27T09:36:50Z

We could wither duplicate the logic, which I don't like. Or, create en external class that both the Base and the Collection use.

Would it be a good idea to create an external Runner class for the run task and have e.g. SingleAnalysisRunner, CollectionAnalysisRunner (I'm not totally sure what the best design pattern is here) ? I'm also thinking about if we plan to implement parallel analysis based on it, it can be simply creating a new ParallelAnalysisRunner (or even ParallelCollectionAnalysisRunner) instead of doing complex inheritance.

PicoCentauri · 2023-02-27T09:44:28Z

Would it be a good idea to create an external Runner class for the run task and have e.g. SingleAnalysisRunner, CollectionAnalysisRunner (I'm not totally sure what the best design pattern is here) ? I'm also thinking about if we plan to implement parallel analysis based on it, it can be simply creating a new ParallelAnalysisRunner (or even ParallelCollectionAnalysisRunner) instead of doing complex inheritance.

I am fine with creating these two classes. To avoid code duplication the SingleAnalysisRunner will inherit from the CollectionAnalysisRunner and just call the run method for a single instance.

Also, should they rather be private classes? I think users will not use them because these runners will require an already prepared analysis class.

hmacdope · 2023-03-05T02:22:40Z

We could wither duplicate the logic, which I don't like. Or, create en external class that both the Base and the Collection use.

I think this solution is the cleanest and doesn't require even more classes which I don't like the idea of. IMO there is already enough of a barrier to entry without every new analysis having to handle 2x new API points.

Personally I would just have an abc abstact base class that they can both inherent from.

hmacdope

Sorry should have left a review.

PicoCentauri · 2023-03-05T10:44:32Z

Yes, we can do an ABC class if the others also agree.

richardjgowers

looking at it, it initially seems strange that Single inherits from Collection, but for the code it does make sense.

@PicoCentauri have you thought about how results could be accessed from the AnalysisCollection object? Or is the intent to always access results from the individual classes again?

richardjgowers · 2023-03-05T13:31:15Z

package/MDAnalysis/analysis/base.py

+        reset_timestep : bool, optional
+            Reset the timestep object after for each ``analysis_object``.
+            Setting this to ``False`` can be useful if an ``analysis_object``
+            is performing a trajectory manipulation which is also useful for the
+            subsequent ``analysis_instances`` e.g. unwrapping of molecules.


on the basis that there should be one way to do anything, I don't like the suggestion that you could use an analysis class to transform data for downstream classes (use a transformation instead). Instead make reset_timestep=True the only option and remove this kwarg

Yes, I agree and I will change this.

richardjgowers · 2023-03-05T14:18:25Z

package/MDAnalysis/analysis/base.py

+        # Ensure compatibility with API of version 0.15.0
+        if not hasattr(self, "_analysis_instances"):
+            self._analysis_instances = (self,)


this is confusing, instead this is making a single analysis class work as a subclass of a Collection right?

No, this is just a leftover from the code before. We can also remove it I think, if we do not support 0.15.0 anymore.

richardjgowers · 2023-03-05T14:21:38Z

package/MDAnalysis/analysis/base.py

@@ -316,6 +489,7 @@ def __init__(self, trajectory, verbose=False, **kwargs):
        self._trajectory = trajectory
        self._verbose = verbose
        self.results = Results()
+        super(AnalysisBase, self).__init__(self)


this ugly super call signature was Python 2.x, I think we can use super().__init__(self) now

richardjgowers · 2023-03-05T14:22:02Z

package/MDAnalysis/analysis/base.py

@@ -220,7 +224,176 @@ def __setstate__(self, state):
        self.data = state


-class AnalysisBase(object):
+class AnalysisCollection(object):


I don't think we need to inherit from object any more either (it's implicit)

richardjgowers · 2023-03-05T14:22:13Z

package/CHANGELOG

@@ -26,6 +25,8 @@ Fixes
    (Issue #3336)

 Enhancements
+  * Add an `AnalayisCollection` class to perform multiple analysis on the same


typo on Analysis

PicoCentauri · 2023-03-23T16:16:05Z

Thanks to all of you for your comments and suggestions and sorry for my later answer.

I agree that the current design is a bit confusing. I thought about the ABC abstract base class. I think this does not really fit here. The reason is that AnalysisCollection works with and on AnalysisBase instances.

A cleaner way for reading the code would be the idea that @yuxuanzhuang suggested that we provide a (private ?) _Runner class that basically is the code of the AnalysisCollection and the setup_frames. This _Runner will analyze one or more AnalysisBase instances. What do you think?

Regarding some questions and comments:

I think this solution is the cleanest and doesn't require even more classes which I don't like the idea of. IMO there is already enough of a barrier to entry without every new analysis having to handle 2x new API points.

This was never the idea. There will only be one API point for new Analysis classes.

@PicoCentauri have you thought about how results could be accessed from the AnalysisCollection object? Or is the intent to always access results from the individual classes again?

The latter. I think accessing the results from the individual instances is already handy. An additional way would be just confusing and blowing up the code.

PicoCentauri · 2023-08-28T11:59:02Z

@orbeckst @hmacdope I would still like to get this feature into main. Before I rebase again I would like to ask for your opinion again. I think the biggest issue to be discussed is the inheritance of AnalysisBase and AnalysisCollection.

orbeckst · 2023-08-28T21:15:39Z

@PicoCentauri sorry for the following short comment... I've run out of "open source time" for the day: There's a longish discussion on parallel analysis in #4158 and it would be good to hear your opinion how the proposed feature could/should work with an AnalysisBase that would try to parallelize with split-apply-combine (see PMDA https://doi.org/10.25080/Majora-7ddc1dd1-013 for the simple idea).

marinegor · 2023-08-31T16:47:40Z

Hi @PicoCentauri , thanks so much for the PR, it's a neat idea!

Regarding of being compatible with #4158, I feel like you'd need to change the _compute method of your AnalysisCollection class instead of run, and the rest will be more or less the same.

And I'd probably indeed subclass AnalysisBase in this case -- you'd need to re-implement __init__, _single_frame, _prepare and _conclude, and run will be then implemented automatically, with an option to be executed on multiple backends too.

PicoCentauri · 2023-09-22T09:32:06Z

Thanks @orbeckst and @marinegor for the comments.

Regarding of being compatible with #4158, I feel like you'd need to change the _compute method of your AnalysisCollection class instead of run, and the rest will be more or less the same.

Changing to _compute is an easy fix and I could do it..

And I'd probably indeed subclass AnalysisBase in this case -- you'd need to re-implement init, _single_frame, _prepare and _conclude, and run will be then implemented automatically, with an option to be executed on multiple backends too.

I am not sure If I get your comment correctly. You would subclass AnalysisBase ? But this means we don't have reimplement the methods. Maybe you meant to NOT subclass AnalysisBase?

marinegor · 2023-09-22T11:59:38Z

@PicoCentauri I indeed mean "subclass AnalysisBase" here, and re-implement the methods that need implementation in subclasses -- otherwise, you need to manually add run method to it, and make sure its semantics is maintained the same with AnalysisBase (I assume this was your intention).

But if you subclass AnalysisBase here, you only need to add _single_frame, _prepare and _conclude methods.
_single_frame would run _single_frame of all analysis objects from the collection, _prepare would initialize their _prepare methods, and _conclude would aggregate the computed results back.

pep8speaks · 2024-06-05T15:32:06Z

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 2024-06-05 16:10:06 UTC

github-actions · 2024-06-05T15:34:14Z

Linter Bot Results:

Hi @PicoCentauri! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/9387570012/job/25850803205

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

PicoCentauri · 2024-06-05T15:48:19Z

Sorry for the lack of updates for a while. I addressed your concerns and moved the implementation of the run method into a standalone function that is used by the AnalysisBase as well as the AnalysisCollection.

marinegor · 2024-08-17T06:53:11Z

hey @PicoCentauri , the parallelization PR that we've mentioned earlier has been merged into develop (#4162), and your AnalysisCollection would be extremely helpful here. Reason is, parallel analysis doesn't provide any real speedup unless processing of a single frame is noticeably slower that reading it from the disk -- but if we're running an AnalysisCollection, it's almost guaranteed!

Could you perhaps have a look at your code after you merge develop into it? I can't yet see how your approach would interact with the current parallelization protocol (you can look at the diagram here: https://docs.mdanalysis.org/dev/documentation_pages/analysis/parallelization.html#split-apply-combine), but it might actually work well.

If this doesn't work, perhaps I'd suggest implementing AnalysisCollection as AnalysisBase subclass, and in its _single_frame running each subclass's _single_frame() and manually updating the trajectory -- this way, you will likely get the parallelization with multiprocessing as soon as you figure out how to aggregate the results. But it's more tedious, and I'm happy to discuss it in more details if you want.

github-actions bot added the Component-Analysis label Jan 30, 2023

PicoCentauri requested review from hmacdope and richardjgowers January 30, 2023 10:15

PicoCentauri force-pushed the analysiscollection branch from 8c2c77d to e4491a7 Compare January 30, 2023 10:17

PicoCentauri force-pushed the analysiscollection branch 3 times, most recently from 924d05c to a6d9ef2 Compare January 30, 2023 14:52

PicoCentauri changed the title ~~Add a AnalayisCollection class~~ Add an AnalayisCollection class Jan 30, 2023

orbeckst changed the title ~~Add an AnalayisCollection class~~ Add an AnalysisCollection class Jan 30, 2023

PicoCentauri force-pushed the analysiscollection branch from bfb6ab8 to a0acb3e Compare January 30, 2023 20:35

richardjgowers approved these changes Feb 15, 2023

View reviewed changes

PicoCentauri force-pushed the analysiscollection branch 2 times, most recently from 0867afd to 3c5de33 Compare February 17, 2023 09:16

orbeckst assigned richardjgowers Feb 24, 2023

orbeckst requested changes Feb 25, 2023

View reviewed changes

hmacdope requested changes Mar 5, 2023

View reviewed changes

richardjgowers reviewed Mar 5, 2023

View reviewed changes

PicoCentauri added 4 commits March 23, 2023 16:36

Add a AnalayisCollection class

f781548

try make linter happy

b58b294

proofread

b71e945

Fix typos and improve docs and code comments

a1b6cdf

Remove reset_timestep option and typos

3ba9d23

PicoCentauri force-pushed the analysiscollection branch from 3c5de33 to 3ba9d23 Compare March 23, 2023 15:46

Merge branch 'develop' into analysiscollection

5e95040

PicoCentauri force-pushed the analysiscollection branch from c03b386 to 04e4967 Compare June 5, 2024 15:42

move run into standalone private function

9ae6a51

PicoCentauri force-pushed the analysiscollection branch from 04e4967 to 9ae6a51 Compare June 5, 2024 15:45

PicoCentauri requested review from orbeckst and hmacdope June 5, 2024 15:50

PicoCentauri added 2 commits June 5, 2024 18:07

remove Self typehint

34a9d73

lint

e762e6a

Add an AnalysisCollection class #4017

Are you sure you want to change the base?

Add an AnalysisCollection class #4017

Conversation

PicoCentauri commented Jan 30, 2023 • edited Loading

PR Checklist

codecov bot commented Jan 30, 2023 • edited Loading

Codecov Report

richardjgowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicoCentauri commented Feb 16, 2023

orbeckst commented Feb 24, 2023

yuxuanzhuang commented Feb 25, 2023

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicoCentauri commented Feb 25, 2023

yuxuanzhuang commented Feb 27, 2023

PicoCentauri commented Feb 27, 2023 • edited Loading

hmacdope commented Mar 5, 2023

hmacdope left a comment

Choose a reason for hiding this comment

PicoCentauri commented Mar 5, 2023

richardjgowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicoCentauri commented Mar 23, 2023

PicoCentauri commented Aug 28, 2023

orbeckst commented Aug 28, 2023

marinegor commented Aug 31, 2023

PicoCentauri commented Sep 22, 2023

marinegor commented Sep 22, 2023

pep8speaks commented Jun 5, 2024 • edited Loading

Comment last updated at 2024-06-05 16:10:06 UTC

github-actions bot commented Jun 5, 2024 • edited Loading

Linter Bot Results:

PicoCentauri commented Jun 5, 2024

marinegor commented Aug 17, 2024

Add an `AnalysisCollection` class #4017

Add an `AnalysisCollection` class #4017

PicoCentauri commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading

PicoCentauri commented Feb 27, 2023 •

edited

Loading

pep8speaks commented Jun 5, 2024 •

edited

Loading

github-actions bot commented Jun 5, 2024 •

edited

Loading