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

lib.distances.transform_StoR doesn't check input type #1699

Closed
richardjgowers opened this Issue Nov 3, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@richardjgowers
Member

richardjgowers commented Nov 3, 2017

The dtype of the box seems to get checked, but not for the S array input, so you get a nonsense answer

Code to reproduce the behaviour

import MDAnalysis as mda

box = np.array([10, 7, 3, 45, 60, 90])
s = np.array([[0.5, -0.1, 0.5]])

box = box.astype(np.float32)

In [9]: mda.lib.distances.transform_StoR(s, box)
Out[9]: array([[  2.62144141e+06,  -1.00000000e-01,   5.00000000e-01]])

In [10]: s.dtype
Out[10]: dtype('float64')

In [15]: s = s.astype(np.float32)

In [16]: s
Out[16]: array([[ 0.5, -0.1,  0.5]], dtype=float32)

In [17]: mda.lib.distances.transform_StoR(s, box)
Out[17]: array([[ 5.75      ,  0.36066014,  0.75000012]], dtype=float32)

Currently version of MDAnalysis:

(run python -c "import MDAnalysis as mda; print(mda.__version__)")

0.17-dev

@palnabarun

This comment has been minimized.

Show comment
Hide comment
@palnabarun

palnabarun Jan 31, 2018

Member

I looked into the code and found that the box is validated by _box_check function. We can have a function which validates the S array too by checking whether the dtype is float32 or not.

Member

palnabarun commented Jan 31, 2018

I looked into the code and found that the box is validated by _box_check function. We can have a function which validates the S array too by checking whether the dtype is float32 or not.

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Jan 31, 2018

Member

@palnabarun Hello! Yeah we've also got a similar function for arrays, it just needs all connecting up and some tests adding to make sure that "bad" arrays are properly rejected

Member

richardjgowers commented Jan 31, 2018

@palnabarun Hello! Yeah we've also got a similar function for arrays, it just needs all connecting up and some tests adding to make sure that "bad" arrays are properly rejected

@palnabarun

This comment has been minimized.

Show comment
Hide comment
@palnabarun

palnabarun Jan 31, 2018

Member

Oh. That's great. I somehow missed reading the function just after _box_check.

I am on it.

Member

palnabarun commented Jan 31, 2018

Oh. That's great. I somehow missed reading the function just after _box_check.

I am on it.

palnabarun added a commit to palnabarun/mdanalysis that referenced this issue Feb 2, 2018

@richardjgowers richardjgowers added this to the 0.17.x milestone Feb 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment