Issue 1829

[CharlieLaughton]

Proposed bugfix for issue #1829

[sukritsingh]

Thanks for the PR submission! Left a couple of comments.

Also - it looks like all the tests passed but could you also run this test for test_angle_180 locally? This way we have peace of mind + have it recorded that it passed prior to merging and behaved as expected

[mattwthompson]

If this angle is meant to be 180 / pi, could we assert that as well? (Or that it's close to pi - pytest.approx is one way to do this)

[CharlieLaughton]

Thought I'd replied to this but obviously not: good idea, thanks for the suggestion, I've added the extra test

[mattwthompson]

To be frank I'm a little surprised a change this potentially far-reaching didn't have any side effects. Then again, it's a poorly-defined edge case so I guess it wasn't hit at all in tests before.

[CharlieLaughton]

Thanks for the PR submission! Left a couple of comments.

Also - it looks like all the tests passed but could you also run this test for test_angle_180 locally? This way we have peace of mind + have it recorded that it passed prior to merging and behaved as expected

No problem - I've run the test locally (pytest -k test_angle_180) and it passes.

[sukritsingh]

No problem - I've run the test locally (pytest -k test_angle_180) and it passes.

Great! I looked through the CI logs for the other test_angles tests (they all passed) but I did notice the following warning:

TypeCastPerformanceWarning: Casting unitcell_vectors dtype=float64 to <class 'numpy.float32'> 
    warnings.warn("Casting %s dtype=%s to %s " % (name, val.dtype, dtype),

Do we think this is something of note? My initial impression is that this simply means we have a little more overhead in dealing with our angles testing but that's relatively minor and I'm not super worried. I need to think about it myself more but I wanted to flag it just in case it was more immediately obvious to others.

[mattwthompson]

Suggested change

	def test_angle_180(get_fn):
	# Test that linear coordinates yield 180 degrees, not "nan"
	a = md.load(get_fn('mol.gro'))
	b = md.compute_angles(a, [[0, 1, 2]])
	@pytest.mark.parametrize("optimize", [True, False])
	def test_angle_180(get_fn, optimize):
	# Test that linear coordinates yield 180 degrees, not "nan"
	a = md.load(get_fn('mol.gro'))
	b = md.compute_angles(a, [[0, 1, 2]], opt=optimize)

IIRC these functions have "NumPy" and "our own C code" pathways - as you modified both - so in principle both paths should be tested

[CharlieLaughton]

Ah - now we get failures due to the default tolerance in pytest.approx (1e-6) being exceeded. If I reduce it to 1e-4, things are OK again. Are you happy to accept this?

[mattwthompson]

... Do we think this is something of note? ...

If it's a new warning, yeah we should avoid that

[CharlieLaughton]

... Do we think this is something of note? ...

If it's a new warning, yeah we should avoid that

I think I have just uncovered something that was already there - that trajectories loaded from Gromacs .gro format files have their unit cell vectors returned as float64s. A non-exhaustive scan through other file formats suggests there is no consistency here - different ones may return float32 or float64s...

[sukritsingh]

If it's a new warning, yeah we should avoid that

Looks like the warning is only coming after the newly inserted test test_angle_180 in this PR, so we should address the warning before merging since it's new:

tests/test_angles.py::test_angle_180[False]
  /home/runner/micromamba/envs/mdtraj-test/lib/python3.12/site-packages/mdtraj/utils/validation.py:115: TypeCastPerformanceWarning: Casting unitcell_vectors dtype=float64 to <class 'numpy.float32'> 
    warnings.warn("Casting %s dtype=%s to %s " % (name, val.dtype, dtype),

trajectories loaded from Gromacs .gro format files have their unit cell vectors returned as float64s. A non-exhaustive scan through other file formats suggests there is no consistency here - different ones may return float32 or float64s...

Oh interesting I wonder if it's due to the precision of the file format being loaded in (and trying to capture as much useful precision as possible). Probably a discussion for a separate issue/PR (but doesn't have to be!)

[CharlieLaughton]

It seems the warning was appearing because in angles.py ensure_type() is used to cast box vectors to np.float32 without the warn_on_cast option being set to False (it defaults to True). Since just a few lines above, it is set to False when the coordinates are checked likewise, I've proposed an edit to match.

[sukritsingh]

Great find! I see now the repeated use of warn_on_cast with the other tests. I think that's fine since it's consistent with the rest of the codebase (and changing it is probably a separate discussion from this.

All the tests pass as expected and no new warning

LGTM!

Assuming @mattwthompson has no additional comments/concerns, I think this is okay for approval/merge?

[sukritsingh]

Fell behind on merging this in but it looks like you might need to master in again @CharlieLaughton and we can see if this still passes! I think this will be important to merge in right away!

[CharlieLaughton]

OK - I have re-synced my fork - please try again!

[sukritsingh]

Hrmmm looks like the conflicts are still there in angles.py and test_angles.py, but it looks like the conflicts are incredibly minor:

In angles.py

<<<<<<< issue_1829
        box = ensure_type(traj.unitcell_vectors, dtype=np.float32, ndim=3, name='unitcell_vectors', shape=(len(xyz), 3, 3), warn_on_cast=False)
=======
        box = ensure_type(
            traj.unitcell_vectors,
            dtype=np.float32,
            ndim=3,
            name="unitcell_vectors",
            shape=(len(xyz), 3, 3),
        )

and in test_angles.py:

<<<<<<< issue_1829
import numpy as np
import pytest
=======
>>>>>>> master

I can resolve these conflicts and it should be relatively easy, but is it easier for you to resolve these on your branch in the PR? If so then want to resolve it on your end?

[mattwthompson]

Those are lints with the exception of the new flag

[sukritsingh]

Those are lints with the exception of the new flag

Oh! Thanks for explaining that!

Hrmm it looks like ubuntu-latest, 3.12 failed at test_distances.py

FAILED tests/test_distance.py::test_compute_distances_core_periodic - ValueError: cannot convert float NaN to integer
FAILED tests/test_distance.py::test_0p - ValueError: cannot convert float NaN to integer
FAILED tests/test_distance.py::test_1p - ValueError: cannot convert float NaN to integer
FAILED tests/test_distance.py::test_2p - ValueError: cannot convert float NaN to integer
FAILED tests/test_distance.py::test_3p - AssertionError: 
Arrays are not almost equal to 6 decimals

x and y nan location mismatch:
 x: array([[8.738824e-01, 1.083976e+00, 8.832984e-01, ..., 1.040335e+00,
        7.327452e-01, 1.243414e+00],
       [1.844674e+19, 1.844674e+19, 1.844674e+19, ..., 1.844674e+19,...
 y: array([[0.873882, 1.083976, 0.883298, ..., 1.040335, 0.732745, 1.243414],
       [     nan,      nan,      nan, ...,      nan,      nan,      nan],
       [0.874888, 0.939474, 0.76201 , ..., 1.822892, 0.955971, 1.985346],...

I'm rerunning the test now in case that was a fluke but otherwise we'll want to see what's going on...

[sukritsingh]

Huh, weird - passed this time.... Assuming no other issues crop up @mattwthompson I'll go ahead and merge this at long last! Thanks again @CharlieLaughton for this great PR!

[mattwthompson]

Stuff's flaky, especially dealing with corner cases like these. I'm not alarmed by 1/N jobs in a matrix needing to run a second time

[CharlieLaughton]

Thanks guys, it's been a pleasure to assist.