Raise an error is parallelizable=True is passed to the NoJump transformation

[p-j-smith]

Expected behavior

There is a comment in the NoJump transformation dunder init:

mdanalysis/package/MDAnalysis/transformations/nojump.py

Lines 106 to 108 in 8923c4a

	# NoJump transforms are inherently unparallelizable, since
	# it depends on the previous frame's unwrapped coordinates
	parallelizable=False,

I would expect that this should be enforced and I should not be able to set parallelizable=True

Actual behavior

However it's not enforced:

mdanalysis/package/MDAnalysis/transformations/nojump.py

Line 110 in 8923c4a

super().__init__(max_threads=max_threads, parallelizable=parallelizable)

and I don't think it's mentioned in the docs. I think it would be good to raise an error if a user sets parallelizable=True.

A more general solution would be to modify TransformationBase to remove the parallelizable argument. A ParallelizableTransformationBase class could then inherit from TransformationBase. This would allow whoever is writing the transformation to decide whether it is parallelizable, and the user calling the transformation could decide whether they want to parallelize it if possible.

Code to reproduce the behavior

import MDAnalysis as mda
from MDAnalysis.tests.datafiles import PSF, DCD,  GRO, PDB, TPR, XTC, TRR,  PRMncdf, NCDF

u = mda.Universe(PSF, DCD)
u.trajectory.add_transformations(mda.transformations.NoJump(parallelizable=True))  # doesn't raise an error

Current version of MDAnalysis

• Which version are you using? (run python -c "import MDAnalysis as mda; print(mda.__version__)")
• Which version of Python (python -V)?
• Which operating system?

[orbeckst]

I agree, it doesn't make sense to have a static property be settable.

I don't think we are currently make use of it but IIRC we had decided that we needed to record if we knew it couldn't work in parallel for the future. (I suppose, the future is @marinegor 's GSOC PR #4162 for parallel analysis — and I am pretty sure that for that one we hadn't thought about having to check for the presence of non-parallelizable transformations).

[marinegor]

and I am pretty sure that for that one we hadn't thought about having to check for the presence of non-parallelizable transformations

yes, I can confirm that I don't check for this.

Am I right that it's enough to run this check in AnalysisBase:

if any((t.parallelizable for t in self._trajectory.transformations)):
  raise ValueError('Trajectory should not have associated parallelizable transformations')

[orbeckst]

That looks correct to me.

[marinegor]

There's probably an automated way to assign that this issue will be fixed by #4162

[RMeli]

@marinegor you can add "fix #4259" on the description of your PR.

[marinegor]

I changed the description of #4604 since it doesn't actually fix what's described here.

I tried to fix it in TODO via just removing parallelizable=... default argument and explicitly passing parallelizable to the super().__init__(...).

[orbeckst]

I agree with the solution in PR #4604 — we don't need to raise an error, we just don't allow passing the useless kwarg in the first place.