Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Input problem in WaterOrientationalRelaxation #2259

Open
alejob opened this issue May 6, 2019 · 3 comments

Comments

@alejob
Copy link
Member

commented May 6, 2019

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 #1293

Why is py3 style division necessary?

Bests,

@alejob alejob changed the title in WaterOrientationalRelaxation Input problem in WaterOrientationalRelaxation May 6, 2019

@alejob

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I'll fix this.

@orbeckst

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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 #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

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.