drop python 3.7 and testing improvements

orbeckst commented 1 year ago

Generic housekeeping PR

drop Python 3.7

[x] fix #248
[x] remove 3.7 from GitHub required status checks

make tests/code more robust

ensure that the workflows directory project_paths are always sorted
make some dihedral tests order independent #239
test of FEP sim that can trigger GMX "perturbed non-bonded interaction beyond cutoff" can now pass, even when condition is randomly triggered #175
allow cleaner testing and error handling for run.runMD_or_exit() and add comprehensive tests

cleanup & updates

replace deprecated nose constructs with slightly less outdated pytest names #254

codecov[bot] commented 1 year ago

Codecov Report

Merging #249 (ab102ed) into develop (edc63b7) will increase coverage by 1.16%. The diff coverage is 90.00%.

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

Impacted Files	Coverage Δ
mdpow/run.py	`71.58% <84.61%> (+7.01%)`	:arrow_up:
mdpow/fep.py	`68.77% <100.00%> (ø)`
mdpow/workflows/base.py	`81.63% <100.00%> (ø)`

... and 7 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

orbeckst commented 1 year ago

@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.

orbeckst commented 1 year ago

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?!

cadeduckworth commented 1 year ago

Could this pertain to using the Mol object for more things now, and explicitly specifying 'center of massvscenter 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: @.***>

cadeduckworth commented 1 year ago

@orbeckst in response to

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 commit bf6884abf1ec81ed705fb578e601d1501519eb57, which merged Develop into that branch.

If that is the case, then those two failing tests are not related to changes you made in this PR.

cadeduckworth commented 1 year ago

Continuation of previous comment:

Develop was last merged into #243 on April 3 with commit 5d8f9601671c4ff50305025d1091801df75d949d and the CI testing report does not indicate that these two new failed tests existed with that merge.

Recent PR History (changes merged into Develop between then and now): #242 was closed on March 27 and #229 was closed on April 3, along with their respectively linked issues.

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 diff to sort this out and I will post what I find here.

cadeduckworth commented 1 year ago

diff.txt

possible notable differences:

sqlalchemy
scipy
python
pytest

cadeduckworth commented 1 year ago

A smarter check with the environments from this PR:

PR_249_ubuntu_latest_py3.8_fail_py3.9_pass_env_diffs.txt

cadeduckworth commented 1 year ago

The set() method might not work well when testing if the atom indices, bond indices, and dihedral groups are correctly paired for highlighting and labeling plots because we cannot test all possible cases, and some changes in ordering might result in incorrect pairing. The current results might still be correct, for now, but having a valid test will be important for any future RDKit changes.

See failed tests in CI report for test_ab_pairs in #243

orbeckst commented 1 year ago

A smarter check with the environments from this PR:

PR_249_ubuntu_latest_py3.8_fail_py3.9_pass_env_diffs.txt

What do you mean by "smarter check"?

orbeckst commented 1 year ago

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.

orbeckst commented 1 year ago

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?

FAILED mdpow/tests/test_automated_dihedral_analysis.py::TestAutomatedDihedralAnalysis::test_ab_pairs - AssertionError: assert {'C10-C5-S4-O...22, 26)), ...} == {'C10-C5-S4-O...21, 26)), ...}
  Differing items:
  {'C2-N3-S4-O12': ((1, 2, 3, 11), (1, 3, 7))} != {'C2-N3-S4-O12': ((1, 2, 3, 11), (4, 2, 0))}
  {'C13-C14-C15-C16': ((12, 13, 14, 15), (19, 22, 25))} != {'C13-C14-C15-C16': ((12, 13, 14, 15), (14, 21, 25))}
  {'C2-C13-C14-C15': ((1, 12, 13, 14), (2, 19, 22))} != {'C2-C13-C14-C15': ((1, 12, 13, 14), (9, 14, 21))}
  {'O1-C2-N3-S4': ((0, 1, 2, 3), (0, 1, 3))} != {'O1-C2-N3-S4': ((0, 1, 2, 3), (8, 4, 2))}
  {'N3-S4-C5-C10': ((2, 3, 4, 9), (3, 5, 9))} != {'N3-S4-C5-C10': ((2, 3, 4, 9), (2, 3, 7))}
  {'N3-C2-C13-C14': ((2, 1, 12, 13), (1, 2, 19))} != {'N3-C2-C13-C14': ((2, 1, 12, 13), (4, 9, 14))}
  {'C6-C5-S4-O11': ((5, 4, 3, 10), (8, 5, 6))} != {'C6-C5-S4-O11': ((5, 4, 3, 10), (6, 3, 1))}
  {'C10-C5-S4-O11': ((9, 4, 3, 10), (9, 5, 6))} != {'C10-C5-S4-O11': ((9, 4, 3, 10), (7, 3, 1))}
  {'N3-S4-C5-C6': ((2, 3, 4, 5), (3, 5, 8))} != {'N3-S4-C5-C6': ((2, 3, 4, 5), (2, 3, 6))}
  {'S4-N3-C2-C13': ((3, 2, 1, 12), (3, 1, 2))} != {'S4-N3-C2-C13': ((3, 2, 1, 12), (2, 4, 9))}
  {'C2-N3-S4-C5': ((1, 2, 3, 4), (1, 3, 5))} != {'C2-N3-S4-C5': ((1, 2, 3, 4), (4, 2, 3))}
  {'C6-C5-S4-O12': ((5, 4, 3, 11), (8, 5, 7))} != {'C6-C5-S4-O12': ((5, 4, 3, 11), (6, 3, 0))}
  {'O1-C2-C13-C14': ((0, 1, 12, 13), (0, 2, 19))} != {'O1-C2-C13-C14': ((0, 1, 12, 13), (8, 9, 14))}
  {'C10-C5-S4-O12': ((9, 4, 3, 11), (9, 5, 7))} != {'C10-C5-S4-O12': ((9, 4, 3, 11), (7, 3, 0))}
  {'C2-N3-S4-O11': ((1, 2, 3, 10), (1, 3, 6))} != {'C2-N3-S4-O11': ((1, 2, 3, 10), (4, 2, 1))}
  {'C13-C14-C15-C20': ((12, 13, 14, 19), (19, 22, 26))} != {'C13-C14-C15-C20': ((12, 13, 14, 19), (14, 21, 26))}
  Full diff:
    {
  -  'C10-C5-S4-O11': ((9, 4, 3, 10), (7, 3, 1)),
  ?                                    ^  ^  ^
  +  'C10-C5-S4-O11': ((9, 4, 3, 10), (9, 5, 6)),
  ?                                    ^  ^  ^
  -  'C10-C5-S4-O12': ((9, 4, 3, 11), (7, 3, 0)),
  ?                                     ------
  +  'C10-C5-S4-O12': ((9, 4, 3, 11), (9, 5, 7)),
  ?                                    ++++++
  -  'C13-C14-C15-C16': ((12, 13, 14, 15), (14, 21, 25)),
  ?                                          ^   ^
  +  'C13-C14-C15-C16': ((12, 13, 14, 15), (19, 22, 25)),
  ?                                          ^   ^
  -  'C13-C14-C15-C20': ((12, 13, 14, 19), (14, 21, 26)),
  ?                                          ^   ^
  +  'C13-C14-C15-C20': ((12, 13, 14, 19), (19, 22, 26)),
  ?                                          ^   ^
  -  'C2-C13-C14-C15': ((1, 12, 13, 14), (9, 14, 21)),
  ?                                          ---- ^
  +  'C2-C13-C14-C15': ((1, 12, 13, 14), (2, 19, 22)),
  ?                                       ++++    ^
  -  'C2-N3-S4-C5': ((1, 2, 3, 4), (4, 2, 3)),
  ?                                 ^  ^  ^
  +  'C2-N3-S4-C5': ((1, 2, 3, 4), (1, 3, 5)),
  ?                                 ^  ^  ^
  -  'C2-N3-S4-O11': ((1, 2, 3, 10), (4, 2, 1)),
  ?                                   ^  ^  ^
  +  'C2-N3-S4-O11': ((1, 2, 3, 10), (1, 3, 6)),
  ?                                   ^  ^  ^
  -  'C2-N3-S4-O12': ((1, 2, 3, 11), (4, 2, 0)),
  ?                                   ^  ^  ^
  +  'C2-N3-S4-O12': ((1, 2, 3, 11), (1, 3, 7)),
  ?                                   ^  ^  ^
  -  'C6-C5-S4-O11': ((5, 4, 3, 10), (6, 3, 1)),
  ?                                    ------
  +  'C6-C5-S4-O11': ((5, 4, 3, 10), (8, 5, 6)),
  ?                                   ++++++
  -  'C6-C5-S4-O12': ((5, 4, 3, 11), (6, 3, 0)),
  ?                                   ^  ^  ^
  +  'C6-C5-S4-O12': ((5, 4, 3, 11), (8, 5, 7)),
  ?                                   ^  ^  ^
  -  'N3-C2-C13-C14': ((2, 1, 12, 13), (4, 9, 14)),
  ?                                     ^   ----
  +  'N3-C2-C13-C14': ((2, 1, 12, 13), (1, 2, 19)),
  ?                                     ^  ++++
  -  'N3-S4-C5-C10': ((2, 3, 4, 9), (2, 3, 7)),
  ?                                  ---   ^
  +  'N3-S4-C5-C10': ((2, 3, 4, 9), (3, 5, 9)),
  ?                                     ^^^^
  -  'N3-S4-C5-C6': ((2, 3, 4, 5), (2, 3, 6)),
  ?                                 ---   ^
  +  'N3-S4-C5-C6': ((2, 3, 4, 5), (3, 5, 8)),
  ?                                    ^^^^
  -  'O1-C2-C13-C14': ((0, 1, 12, 13), (8, 9, 14)),
  ?                                     ^   ----
  +  'O1-C2-C13-C14': ((0, 1, 12, 13), (0, 2, 19)),
  ?                                     ^  ++++
  -  'O1-C2-N3-S4': ((0, 1, 2, 3), (8, 4, 2)),
  ?                                 ^  ^  ^
  +  'O1-C2-N3-S4': ((0, 1, 2, 3), (0, 1, 3)),
  ?                                 ^  ^  ^
  -  'S4-N3-C2-C13': ((3, 2, 1, 12), (2, 4, 9)),
  ?                                    ------
  +  'S4-N3-C2-C13': ((3, 2, 1, 12), (3, 1, 2)),
  ?                                   ++++++
    }

cadeduckworth commented 1 year ago

A smarter check with the environments from this PR: PR_249_ubuntu_latest_py3.8_fail_py3.9_pass_env_diffs.txt

What do you mean by "smarter check"?

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.

cadeduckworth commented 1 year ago

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?

FAILED mdpow/tests/test_automated_dihedral_analysis.py::TestAutomatedDihedralAnalysis::test_ab_pairs - AssertionError: assert {'C10-C5-S4-O...22, 26)), ...} == {'C10-C5-S4-O...21, 26)), ...}
  Differing items:
  {'C2-N3-S4-O12': ((1, 2, 3, 11), (1, 3, 7))} != {'C2-N3-S4-O12': ((1, 2, 3, 11), (4, 2, 0))}
  {'C13-C14-C15-C16': ((12, 13, 14, 15), (19, 22, 25))} != {'C13-C14-C15-C16': ((12, 13, 14, 15), (14, 21, 25))}
  {'C2-C13-C14-C15': ((1, 12, 13, 14), (2, 19, 22))} != {'C2-C13-C14-C15': ((1, 12, 13, 14), (9, 14, 21))}
  {'O1-C2-N3-S4': ((0, 1, 2, 3), (0, 1, 3))} != {'O1-C2-N3-S4': ((0, 1, 2, 3), (8, 4, 2))}
  {'N3-S4-C5-C10': ((2, 3, 4, 9), (3, 5, 9))} != {'N3-S4-C5-C10': ((2, 3, 4, 9), (2, 3, 7))}
  {'N3-C2-C13-C14': ((2, 1, 12, 13), (1, 2, 19))} != {'N3-C2-C13-C14': ((2, 1, 12, 13), (4, 9, 14))}
  {'C6-C5-S4-O11': ((5, 4, 3, 10), (8, 5, 6))} != {'C6-C5-S4-O11': ((5, 4, 3, 10), (6, 3, 1))}
  {'C10-C5-S4-O11': ((9, 4, 3, 10), (9, 5, 6))} != {'C10-C5-S4-O11': ((9, 4, 3, 10), (7, 3, 1))}
  {'N3-S4-C5-C6': ((2, 3, 4, 5), (3, 5, 8))} != {'N3-S4-C5-C6': ((2, 3, 4, 5), (2, 3, 6))}
  {'S4-N3-C2-C13': ((3, 2, 1, 12), (3, 1, 2))} != {'S4-N3-C2-C13': ((3, 2, 1, 12), (2, 4, 9))}
  {'C2-N3-S4-C5': ((1, 2, 3, 4), (1, 3, 5))} != {'C2-N3-S4-C5': ((1, 2, 3, 4), (4, 2, 3))}
  {'C6-C5-S4-O12': ((5, 4, 3, 11), (8, 5, 7))} != {'C6-C5-S4-O12': ((5, 4, 3, 11), (6, 3, 0))}
  {'O1-C2-C13-C14': ((0, 1, 12, 13), (0, 2, 19))} != {'O1-C2-C13-C14': ((0, 1, 12, 13), (8, 9, 14))}
  {'C10-C5-S4-O12': ((9, 4, 3, 11), (9, 5, 7))} != {'C10-C5-S4-O12': ((9, 4, 3, 11), (7, 3, 0))}
  {'C2-N3-S4-O11': ((1, 2, 3, 10), (1, 3, 6))} != {'C2-N3-S4-O11': ((1, 2, 3, 10), (4, 2, 1))}
  {'C13-C14-C15-C20': ((12, 13, 14, 19), (19, 22, 26))} != {'C13-C14-C15-C20': ((12, 13, 14, 19), (14, 21, 26))}
  Full diff:
    {
  -  'C10-C5-S4-O11': ((9, 4, 3, 10), (7, 3, 1)),
  ?                                    ^  ^  ^
  +  'C10-C5-S4-O11': ((9, 4, 3, 10), (9, 5, 6)),
  ?                                    ^  ^  ^
  -  'C10-C5-S4-O12': ((9, 4, 3, 11), (7, 3, 0)),
  ?                                     ------
  +  'C10-C5-S4-O12': ((9, 4, 3, 11), (9, 5, 7)),
  ?                                    ++++++
  -  'C13-C14-C15-C16': ((12, 13, 14, 15), (14, 21, 25)),
  ?                                          ^   ^
  +  'C13-C14-C15-C16': ((12, 13, 14, 15), (19, 22, 25)),
  ?                                          ^   ^
  -  'C13-C14-C15-C20': ((12, 13, 14, 19), (14, 21, 26)),
  ?                                          ^   ^
  +  'C13-C14-C15-C20': ((12, 13, 14, 19), (19, 22, 26)),
  ?                                          ^   ^
  -  'C2-C13-C14-C15': ((1, 12, 13, 14), (9, 14, 21)),
  ?                                          ---- ^
  +  'C2-C13-C14-C15': ((1, 12, 13, 14), (2, 19, 22)),
  ?                                       ++++    ^
  -  'C2-N3-S4-C5': ((1, 2, 3, 4), (4, 2, 3)),
  ?                                 ^  ^  ^
  +  'C2-N3-S4-C5': ((1, 2, 3, 4), (1, 3, 5)),
  ?                                 ^  ^  ^
  -  'C2-N3-S4-O11': ((1, 2, 3, 10), (4, 2, 1)),
  ?                                   ^  ^  ^
  +  'C2-N3-S4-O11': ((1, 2, 3, 10), (1, 3, 6)),
  ?                                   ^  ^  ^
  -  'C2-N3-S4-O12': ((1, 2, 3, 11), (4, 2, 0)),
  ?                                   ^  ^  ^
  +  'C2-N3-S4-O12': ((1, 2, 3, 11), (1, 3, 7)),
  ?                                   ^  ^  ^
  -  'C6-C5-S4-O11': ((5, 4, 3, 10), (6, 3, 1)),
  ?                                    ------
  +  'C6-C5-S4-O11': ((5, 4, 3, 10), (8, 5, 6)),
  ?                                   ++++++
  -  'C6-C5-S4-O12': ((5, 4, 3, 11), (6, 3, 0)),
  ?                                   ^  ^  ^
  +  'C6-C5-S4-O12': ((5, 4, 3, 11), (8, 5, 7)),
  ?                                   ^  ^  ^
  -  'N3-C2-C13-C14': ((2, 1, 12, 13), (4, 9, 14)),
  ?                                     ^   ----
  +  'N3-C2-C13-C14': ((2, 1, 12, 13), (1, 2, 19)),
  ?                                     ^  ++++
  -  'N3-S4-C5-C10': ((2, 3, 4, 9), (2, 3, 7)),
  ?                                  ---   ^
  +  'N3-S4-C5-C10': ((2, 3, 4, 9), (3, 5, 9)),
  ?                                     ^^^^
  -  'N3-S4-C5-C6': ((2, 3, 4, 5), (2, 3, 6)),
  ?                                 ---   ^
  +  'N3-S4-C5-C6': ((2, 3, 4, 5), (3, 5, 8)),
  ?                                    ^^^^
  -  'O1-C2-C13-C14': ((0, 1, 12, 13), (8, 9, 14)),
  ?                                     ^   ----
  +  'O1-C2-C13-C14': ((0, 1, 12, 13), (0, 2, 19)),
  ?                                     ^  ++++
  -  'O1-C2-N3-S4': ((0, 1, 2, 3), (8, 4, 2)),
  ?                                 ^  ^  ^
  +  'O1-C2-N3-S4': ((0, 1, 2, 3), (0, 1, 3)),
  ?                                 ^  ^  ^
  -  'S4-N3-C2-C13': ((3, 2, 1, 12), (2, 4, 9)),
  ?                                    ------
  +  'S4-N3-C2-C13': ((3, 2, 1, 12), (3, 1, 2)),
  ?                                   ++++++
    }

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.

orbeckst commented 1 year ago

I created two local envs on osX for 3.8 and 3.10 and

pytest -v mdpow/tests/test_analysis.py -k test_TI

passes in both envs.

orbeckst commented 1 year ago

Hi @cadeduckworth, from https://github.com/Becksteinlab/MDPOW/pull/249#issuecomment-1612390653

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

cadeduckworth commented 1 year ago

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.

Becksteinlab / MDPOW