drop python 3.7 and testing improvements #249
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #249 +/- ##
===========================================
+ Coverage 79.53% 80.69% +1.16%
===========================================
Files 15 15
Lines 1881 1906 +25
Branches 277 294 +17
===========================================
+ Hits 1496 1538 +42
+ Misses 293 276 -17
Partials 92 92
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@cadeduckworth please review, in particular the fixes to the automated dihedral tests where I am using sets to make them insensitive to the order. I also ensured ordering of project_dirs. |
|
The dihedral tests now all pass. I am not sure why the other tests fail. The differences are small-ish... not sure what changed?! |
|
Could this pertain to using the Mol object for more things now, and
explicitly specifying 'center of mass` vs `center of geometry`, or are
these values derived strictly from the trajectory? I will check back in a
minute, but I am only seeing what is shown in the email at the moment.
…On Tue, Jun 27, 2023 at 8:03 PM Oliver Beckstein ***@***.***> wrote:
The dihedral tests now all pass. I am not sure why the other tests
FAILED mdpow/tests/test_analysis.py::TestAnalyze::test_convert_edr - AssertionError:
Arrays are not almost equal to 5 decimals
Mismatched elements: 1 / 2 (50%)
Max absolute difference: 0.00028187
Max relative difference: 0.00012179
x: array([-3.72175, 2.31415])
y: array([-3.72175, 2.31443])
FAILED mdpow/tests/test_analysis.py::TestAnalyze::test_TI - AssertionError:
Arrays are not almost equal to 5 decimals
Mismatched elements: 1 / 2 (50%)
Max absolute difference: 0.00028187
Max relative difference: 0.00012179
x: array([-3.72175, 2.31415])
y: array([-3.72175, 2.31443])
fail.
The differences are small-ish... not sure what changed?!
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/Becksteinlab/MDPOW/pull/249*issuecomment-1610425960__;Iw!!IKRxdwAv5BmarQ!d_zbTEa7ex5HezGCdLdb55Mtk08rYWMMfU67dQ6vjmR9egoP3XMNo3YEfffrjVnfJp44bhRFq79MBX2cn5ZcMQs_$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AY4TYYDARS5CWW5HOOAETH3XNN7EFANCNFSM6AAAAAAZWH7CT4__;!!IKRxdwAv5BmarQ!d_zbTEa7ex5HezGCdLdb55Mtk08rYWMMfU67dQ6vjmR9egoP3XMNo3YEfffrjVnfJp44bhRFq79MBX2cn8wjGF-q$>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| @@ -22,7 +19,7 @@ | |||
|
|
|||
| import py.path | |||
|
|
|||
| from ..workflows import dihedrals | |||
| from mdpow.workflows import dihedrals | |||
There was a problem hiding this comment.
Did the CI give you any trouble using this import method?
There was a problem hiding this comment.
No, but the tests should use an installed version of the package instead of the files that sit somewhere next to the tests.
|
How can I tell where the starting code for this PR is branched from before changes? After checking #243, it seems the same failed tests are showing up with If that is the case, then those two failing tests are not related to changes you made in this PR. |
|
Continuation of previous comment:
Recent PR History (changes merged into This PR, #249 was started (3 hours ago) after the aforementioned merge and failed tests showed up in #243 (4 hours ago), which supports the idea that any changes made in this PR are not likely responsible for the new failed tests. My best guess is that this is a new version issue that we will need to sort out. I am going to try to use |
|
possible notable differences:
|
|
A smarter check with the environments from this PR: |
|
The See failed tests in CI report for |
What do you mean by "smarter check"? |
|
I think the main problem is that we are assuming that RDKIT always returns indices in one specific order. If we want this then we should sort the indices ourselves. If we don't want to do sorting then you could rewrite the tests so that they simply check all combinations that we have seen so far and if one matches, it's ok. |
|
I see, https://github.com/Becksteinlab/MDPOW/actions/runs/5395344961/jobs/9797708880?pr=243#step:9:4924 looks more different than just reordering. Which one is correct? |
For the first check of differences, I forgot that PR #243 still has a commit that specifies the version of RDKit, which might affect other dependencies. Then I decided it made more sense to check for differences between the most similar cases where the failed tests first occur, in this PR. |
Hey Oliver, Whenever I generate the comparison values for tests I usually do it in a notebook to have a record of how those values were obtained. if I have time I will try to review this one before our meeting tomorrow. It is my understanding that the order the Mol object is 'constructed' has changed. For some of the more symmetrical molecules this might appear to be a reordering if there are similar dihedral atom groups, when it is actually a new indexing scheme. For example (hypothetical), C1-C2-C3-O4 and O12-C1-C2-C3 could give incorrect or misleading results if O4 and O12 are indexed in the opposite direction. I think I remember reading it is based on the types of bonds, which determine the direction in which the Mol object is constructed and indexed. |
|
I created two local envs on osX for 3.8 and 3.10 and passes in both envs. |
|
Hi @cadeduckworth, from #249 (comment)
Are you saying that RDKIT mol object may contain atoms in a different order from the MDAnalysis Universe? That would be bad because we are relying on being able to do mol = u.atoms.convert_to("RDKIT")
atom_indices = mol.GetSubstructMatches(pattern)
ag = u.atoms[atom_indices[0]] |
|
The construction of the Mol object during the RDKit conversion from the MDAnalysis Universe is when the atoms and bonds are indexed, and those indices are used for the subsequent duration of the analysis. See the links in PR #243 for the RDKit PR and comments describing the indexing changes during conversion. We should look into this during our meeting to make sure the resulting dihedrals are still being labeled properly now.
… On Jun 29, 2023, at 1:45 PM, Oliver Beckstein ***@***.***> wrote:
Hi @cadeduckworth, from #249 (comment)
It is my understanding that the order the Mol object is 'constructed' has changed.
Are you saying that RDKIT mol object may contain atoms in a different order from the MDAnalysis Universe? That would be bad because we are relying on being able to do
mol = u.atoms.convert_to("RDKIT")
atom_indices = mol.GetSubstructMatches(pattern)
ag = u.atoms[atom_indices[0]]
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
orbeckst
force-pushed
the
drop-37
branch
2 times, most recently
from
d6ed490 to
62d7fa5
Compare
- use set comparisons - removed skipif for python < 3.8 and replaced with comment referencing issue #239
- On CI, TestAnalyze.test_TI failed for Python > 3.8 and the computed values for the free energy error estimate differed from reference values. The free energies themselves remained the same. These error estimates are computed in a complicated manner from error propagation through simpsons rule via numkit/scipy. It is possible that there are subtle changes. However, this is not very important functionality anymore because we use alchemlyb. - Reduced comparison precision to decimals=3 to make the tests pass robustly. - Locally on macOS, @orbeckst could not reproduce the different values (they always came out exactly as the original reference, regardless of working in a 3.8 or 3.10 environment)
- add exit_on_error=True kwarg to make it possible to just raise exceptions or return values instead of sys.exit; default True is old behavior - no real testing
- add new kwarg exit_on_error=True|False to run runMD_or_exit() to only raise exceptions instead of calling sys.exit(); default is old behavior (True) - added tests for runMD_or_exit() failure modes - use better error handling in runMD_or_exit to check if we triggered the non-bonded interactions beyond cutoff issue and if so, pass with xfail - close #175
- close #254 - renamed setup -> setup_method and teardown -> teardown_method but ultimately these tests should be rewritten with fixtures - fixed relative imports from package in tests: mdpow imports should be from the installed package
Generic housekeeping PR
drop Python 3.7
make tests/code more robust
run.runMD_or_exit()and add comprehensive testscleanup & updates