Difference between self._sliced_trajectory and self._trajectory in analysis classes

[marinegor]

Hi everyone,

I wonder what's the difference between self._trajectory (that gets initialized in AnalysisBase.__init__) and self._sliced_trajectory (initialized in _setup_frames), and which should be used in which case?

The way I see it, is that self._trajectory should basically never be used in analysis runs, since it doesn't hold the information about start, stop, step or frames arguments, passed to run() method. However, I sometimes see subclasses using directly self._trajectory (e.g. diffusionmap.DistanceMatrix or pca.PCA). What's the rationale for that?

Tagging @hmacdope and @IAlibay since it continues the discussion in PR #4423

[IAlibay]

Off the top of my head, the likely reasoning here is that self._sliced_trajectory is a new addition to AnalysisBase (well 2 years new), which means that a lot of the analysis classes haven't been switched over.

I'm going to tag @orbeckst here who might have a better recollection of all this.

[orbeckst]

I think you're both correct.

Did you look at the git-blame log to find out where we implemented the change? The corresponding PR might have additional information.

We should document internal usage — comments and docs.

[marinegor]

It was @rsexton2 in #3415 but I can't see the description of _sliced_trajectory (and reasons why other classes don't use it).

I now assume that all subclasses should not touch self._trajectory at all, right? Meaning, all these:

and these:

And this is probably the reason for the bug #4423 (and few others when analysis is run with explicit frames)?

[orbeckst]

I think I agree with your assessment.

Looking at #3415, it looks to me that other analysis tools should be using self._sliced_trajectory instead of self._trajectory[start:stop:step] to be able to use frames or start/stop/step. This probably only affects analysis classes that do more than implementing _single_frame() and instead do their own thing.

[marinegor]

Ok, so I'll probably add a separate PR fixing it in all but PCA subclasses, after #4423 is merged.

[marinegor]

Separate PR added: #4433 (turned out only diffusionmap needs that)