search

MDAnalysis / waterdynamics

Analysis of water dynamics in molecular dynamics trajectories and water interactions with other molecules.
https://mdanalysis.org/waterdynamics
GNU General Public License v2.0
5 stars 4 forks source link

Input problem in WaterOrientationalRelaxation #23

Open alejob opened 5 years ago

alejob commented 5 years ago

Expected behavior To crash if not a water selection is given.

Actual behavior It calculates with any input, not only with water molecules.

Code to reproduce the behavior

I'm giving a selection of only oxygens, and it should crash, but it doesn't.

import MDAnalysis
from MDAnalysis.analysis.waterdynamics import WaterOrientationalRelaxation as WOR

u = MDAnalysis.Universe(pdb, trajectory)
selection = "name OH2 and sphzone 6.0 protein and resid 42"
WOR_analysis = WOR(universe, selection, 0, 1000, 20)
WOR_analysis.run()
....

I've just realize that this PR create the error MDAnalysis/mdanalysis#1293

Why is py3 style division necessary?

Bests,

alejob commented 5 years ago

I'll fix this.

orbeckst commented 5 years ago

Why is py3 style division necessary?

At the moment MDA needs to run under Py 2.7 and Py 3.5+. So all files need to use from __future__ import division which makes the division operator / always perform floating point division. If you want the old integer division behavior then you need to be explicit about it and use //.

It is possible that PR MDAnalysis/mdanalysis#1293 wrongly replaced integer division with floating point division. Such things should be caught by tests and code review but sometimes the tests do not cover a particular corner case and if the reviewer is not the original author of the code then such things can slip through.

That's why we insist that code authors write tests that cover their code well because it's hard to maintain code when the original authors are not responsive to reviewing PRs any more.

orbeckst commented 5 years ago

By the way, I hope by "crashing" you mean "raise exception with meaningful error message".

zemanj commented 5 years ago

Expected behavior To crash if not a water selection is given.

Actual behavior It calculates with any input, not only with water molecules.

Given the fact that atom and residue names can be freely defined, and that mass, charge, or bond information is missing in several supported topology formats, IMHO it is a very bad idea to be smart and try to determine the chemical compounds a selection contains. User input varies widely, making it impossible to unambiguously interpret the contents of a selection, even if it's just such a "simple" case like water. Furthermore, there exists a plethora of different water models, which makes their identification even harder.

What could (and probably should) be done is to improve the docs by clearly stating that it is up to the user to supply valid input for a given algorithm.

@orbeckst

By the way, I hope by "crashing" you mean "raise exception with meaningful error message".

If I want to use WaterOrientationalRelaxation with other, apparently non-water molecules, I'd be quite frustrated if MDAnalysis wouldn't let me do that.