distopia 0.3.0 compatibility changes #4734
Conversation
|
Hello @hmacdope! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-24 03:42:25 UTC |
Linter Bot Results:Hi @hmacdope! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4734 +/- ##
===========================================
- Coverage 93.65% 86.21% -7.44%
===========================================
Files 175 187 +12
Lines 21564 22676 +1112
Branches 3023 3030 +7
===========================================
- Hits 20195 19550 -645
- Misses 925 2690 +1765
+ Partials 444 436 -8 ☔ View full report in Codecov by Sentry. |
|
@orbeckst @richardjgowers this is ready for a first look over. The failing tests are for a 180 degree dihedral where numpy returns np.pi and distopia returns -np.pi (equivalent in polar coordinates). Are we ok to change the test to account for this? There are also options for changing in distopia but at the cost of a lot of performance improvement. |
|
I'd be ok with changing tests but adding a note to docs. |
|
FYI, upstream distopia 0.3.0 release is currently breaking all tests, see #4739 . |
Co-authored-by: Oliver Beckstein <[email protected]>
hmacdope
changed the title
There was a problem hiding this comment.
Awesome to also have support for distance_array/self_distance_array!
I have a few comments on the docs (see inline) and minor suggestion for the test.
My major issue at the moment is that codecov does not mark the distance_array functions as tested. Given that it sees the bonds and angles, I wonder if we are really testing distopia-accelerated distance_array. Can you please double check? Thanks!
| import MDAnalysis.lib._distopia | ||
| assert not MDAnalysis.lib._distopia.HAS_DISTOPIA | ||
| sys.modules.pop("MDAnalysis.lib._distopia", None) | ||
| if HAS_DISTOPIA: |
There was a problem hiding this comment.
For the test I'd ignore HAS_DISTOPIA and not add the conditional. In principle we always want to test explicitly that the version check is working. By using the conditional you implicitly assume that HAS_DISTOPIA was set to False because of the version check.
| assert not MDAnalysis.lib._distopia.HAS_DISTOPIA | ||
| sys.modules.pop("MDAnalysis.lib._distopia", None) | ||
| if HAS_DISTOPIA: | ||
| with patch('distopia.__version__', '0.1.0'): |
There was a problem hiding this comment.
This only works if we can actually import distopia so I now understand why you need the conditional.
I'd prefer the test to work regardless of distopia being present, which can be achieved with the previous approach of creating a mocked module.
background on patch()
(I had to read up on patch to understand how it does its magic so in case anyone else is interested, here's the background.)
The docs for patch() say
unittest.mock.patch(target, new=DEFAULT, spec=None, create=False, spec_set=None, autospec=None, new_callable=None, **kwargs)
...
target should be a string in the form 'package.module.ClassName'. The target is imported and the specified object replaced with the new object, so the target must be importable from the environment you are calling patch() from.
(my emphasis).
Just to confirm: patch needs an importable module if one relies on the default behavior:
In [2]: from unittest.mock import patch
In [3]: import distopia
---------------------------------------------------------------------------
ModuleNotFoundError Traceback (most recent call last)
Cell In[3], line 1
----> 1 import distopia
ModuleNotFoundError: No module named 'distopia'
In [4]: with patch('distopia.__version__', '0.1.0'):
...: import distopia
...: print(distopa.__version__)
...:
---------------------------------------------------------------------------
ModuleNotFoundError Traceback (most recent call last)
Cell In[4], line 1
----> 1 with patch('distopia.__version__', '0.1.0'):
2 import distopia
3 print(distopa.__version__)
...
ModuleNotFoundError: No module named 'distopia'| ) -> None: | ||
| distopia.calc_self_distance_array_no_box(coords, results=results) | ||
|
|
||
| def calc_self_distance_array_ortho( |
There was a problem hiding this comment.
Not tested + docs of self_distance_array
Co-authored-by: Oliver Beckstein <[email protected]>
| @@ -946,6 +939,19 @@ def test_bonds(self, box, backend, dtype, pos, request): | |||
| assert_almost_equal(dists_pbc[3], 3.46410072, self.prec, | |||
| err_msg="PBC check #w with box") | |||
|
|
|||
| @pytest.mark.parametrize("dtype", (np.float32, np.float64)) | |||
| @pytest.mark.parametrize("backend", distopia_conditional_backend()) | |||
| def test_results_inplace_all_backends(self, backend, dtype,): | |||
| @@ -303,6 +312,14 @@ def distance_array(reference: Union[npt.NDArray, 'AtomGroup'], | |||
| distances = _check_result_array(result, (refnum, confnum)) | |||
| if len(distances) == 0: | |||
| return distances | |||
|
|
|||
There was a problem hiding this comment.
@orbeckst @richardjgowers we have an issue which is that the distopia backend doesn't seem to fill the result array on return. ie post call for the other backends, result == distances outside the function while for distopia its passed through. Not sure if I am doing something simple wrong.
Offending example in a test on this PR.
@pytest.mark.parametrize("dtype", (np.float32, np.float64))
@pytest.mark.parametrize("backend", distopia_conditional_backend())
def test_results_inplace_all_backends(self, backend, dtype,):
N = 10
c0 = np.ones(3 * N, dtype=dtype).reshape(N, 3) * 2
c1 = np.ones(3 * N, dtype=dtype).reshape(N, 3) * 3
result = np.zeros(N, dtype=np.float64)
distances.calc_bonds(c0, c1, result=result, backend=backend)
expected = np.ones(N, dtype=dtype) * 3**(1/2)
# test the result array is updated in place
assert_almost_equal(result, expected, self.prec, err_msg="calc_bonds inplace failed")We can get around this with a copy, but shouldn't be nessecary right? The distopia core API works inplace. Im fairly sure I am just getting some basic scoping wrong.
There was a problem hiding this comment.
This test shows distopia works in place.
There was a problem hiding this comment.
I'd look into any use of @check_coords as it creates copies by default.
There was a problem hiding this comment.
(Maybe not relevant as you're concerned with results.)
There was a problem hiding this comment.
I think it's actually the astype() — see comment review.
| HAS_DISTOPIA = False | ||
|
|
||
|
|
||
| from .c_distances import ( | ||
| calc_bond_distance_triclinic as _calc_bond_distance_triclinic_serial, | ||
| ) | ||
| import numpy as np | ||
|
|
||
|
|
||
| def calc_bond_distance_ortho( | ||
| coords1, coords2: np.ndarray, box: np.ndarray, results: np.ndarray |
There was a problem hiding this comment.
Any good reason why coords1 does not have a type?
| # distopia requires that all the input arrays are the same type, | ||
| # while MDAnalysis allows for mixed types, this should be changed | ||
| # pre 0.3.0 release see issue #3707 | ||
| distances = distances.astype(np.float32) |
| if backend == 'distopia': | ||
| # mda expects the result to be in float64, so we need to convert it back | ||
| # to float64, change for 3.0, see #3707 | ||
| distances = distances.astype(np.float64) |
There was a problem hiding this comment.
You may have to instead put the numbers back into the original array to make in-place work.
result[:] = distances
...
return resultor something of that kind.
|
@hmacdope did you look further into the copy vs in-place issue? |
Test implementation of distopia-0.3.0 distances API.
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4734.org.readthedocs.build/en/4734/