add function to create a minimal network with redundancy for all nodes #559

JenkeScheen · 2023-09-15T13:36:49Z

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

I'm building a function that will generate a LigandNetwork where each node is connected to two edges. This will increase performance during analysis by introducing redundancy: if one of the two edges has a high error associated with it, the other edge (hopefully with much lower error) will be weighted to contribute more to the DG estimation.

The basic algorithm will be based on openfe.setup.ligand_network_planning.generate_minimal_spanning_network, but will also add the 'second-best-scoring` edge for each node.

richardjgowers · 2023-09-15T18:20:29Z

This sounds handy. I'm curious if doubly connected MST is rigorously defined, but I'm sure there's ways to approximate it. One hacky way would be to run MST, set the chosen edges to bad values then rerun the MST alg to get the "2nd best" network.

codecov · 2023-09-15T18:27:11Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ab0ae26) 91.29% compared to head (578b7a1) 91.38%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
+ Coverage   91.29%   91.38%   +0.08%     
==========================================
  Files         117      117              
  Lines        7287     7357      +70     
==========================================
+ Hits         6653     6723      +70     
  Misses        634      634

Files	Coverage Δ
openfe/tests/setup/test_network_planning.py	`100.00% <100.00%> (ø)`
openfecli/commands/gather.py	`85.98% <100.00%> (+0.73%)`	⬆️
openfecli/tests/commands/test_gather.py	`100.00% <100.00%> (ø)`
openfe/setup/ligand_network_planning.py	`99.30% <95.00%> (-0.70%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JenkeScheen · 2023-09-18T08:28:55Z

This sounds handy. I'm curious if doubly connected MST is rigorously defined, but I'm sure there's ways to approximate it. One hacky way would be to run MST, set the chosen edges to bad values then rerun the MST alg to get the "2nd best" network.

doing something like this. I'm currently just running an MST, then removing those edges from the fully-connected network, then running MST again, then adding the two MSTs together.

JenkeScheen · 2023-09-18T09:35:28Z

@richardjgowers this is working for me locally, so ready for review. I'm not familiar with how you've set up testing, should I add tests for this function somewhere?

A simple test would be to assert that all nodes are connected to n edges, but I'm not sure how to expose the networkx object from a LigandNetwork?

richardjgowers · 2023-09-18T09:41:55Z

@JenkeScheen LigandNetwork.graph should give you a nx.MultiDiGraph. This file has the relevant tests: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/tests/setup/test_network_planning.py

JenkeScheen · 2023-09-18T13:47:08Z

added a test that all nodes are connected to at least two edges. @richardjgowers please fiddle with the tests as you see fit, the copy-pasting from test_minimal_spanning_network isn't very elegant. Other than that this is ready to be reviewed fully.

dwhswenson

Thanks! This is a great contribution.

One definite fix needed: looks like a test isn't being prefaced by test_, so it isn't running.

Other suggestions are mainly potential improvements to the tests.

openfe/setup/ligand_network_planning.py

openfe/tests/setup/test_network_planning.py

pep8speaks · 2023-09-20T20:25:20Z

Hello @JenkeScheen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file openfe/setup/ligand_network_planning.py:

Line 241:1: W293 blank line contains whitespace
Line 242:78: W291 trailing whitespace
Line 243:68: W291 trailing whitespace
Line 284:80: E501 line too long (80 > 79 characters)

In the file openfe/tests/setup/test_network_planning.py:

Line 258:80: E501 line too long (86 > 79 characters)
Line 267:80: E501 line too long (81 > 79 characters)
Line 292:80: E501 line too long (92 > 79 characters)
Line 293:80: E501 line too long (89 > 79 characters)
Line 343:80: E501 line too long (90 > 79 characters)

Comment last updated at 2023-10-10 08:28:18 UTC

openfe/setup/ligand_network_planning.py

…edge_data['object']) for _, _, _, edge_data in current_best_edges] for _, _, _, edge_data in current_best_edges: mappings.append(edge_data['object']) Don't use a list comprehension unless you're assigning it to something. Co-authored-by: David W.H. Swenson <[email protected]>

…t_network

JenkeScheen · 2023-09-21T09:06:50Z

Thanks for the review @dwhswenson, should be ready for another look now.

mikemhenry · 2023-09-21T21:35:11Z

@JenkeScheen looks like CI is unhappy

JenkeScheen · 2023-09-22T07:42:30Z

thanks @mikemhenry, should be good now.

mikemhenry · 2023-09-22T18:21:49Z

now mypy needs some tlc https://github.com/OpenFreeEnergy/openfe/actions/runs/6271512074/job/17031266889?pr=559#step:6:13
openfe/tests/setup/test_network_planning.py:299: error: Name "test_minimal_redundant_network" already defined on line 267 [no-redef]

mikemhenry · 2023-09-26T17:42:57Z

Thanks @JenkeScheen

This week most of us are busy at the MDAnalysis UGM, so if this doesn't get a review this week, I will be sure to get some eyes on it next Wednesday (10/04)

mikemhenry · 2023-10-05T20:07:15Z

These mypy errors are coming in from files not in in this PR, so I am guessing that something got merged in either without mypy passing, or there was an update to mypy so we have a new error to fix...

openfe/protocols/openmm_utils/multistate_analysis.py:77: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:77: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:86: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:86: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:95: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:95: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:105: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:105: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]

IAlibay · 2023-10-05T22:53:02Z

These mypy errors are coming in from files not in in this PR, so I am guessing that something got merged in either without mypy passing, or there was an update to mypy so we have a new error to fix...

openfe/protocols/openmm_utils/multistate_analysis.py:77: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:77: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:86: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:86: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:95: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:95: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:105: error: Item "SubFigure" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]
openfe/protocols/openmm_utils/multistate_analysis.py:105: error: Item "None" of "Union[Figure, SubFigure, None]" has no attribute "savefig"  [union-attr]

Yeah this one just suddenly appeared it seems.

IAlibay · 2023-10-05T22:56:24Z

Looks like it came along with matplotlib 3.8.0 matplotlib/matplotlib#26812

JenkeScheen added 2 commits September 15, 2023 15:29

add empty function with adjusted docstring

b43c153

remove bulk of function, will add back gradually.

a91f92d

JenkeScheen mentioned this pull request Sep 18, 2023

an option to make generated networks have redundancy, i.e. all ligand nodes should be in a cycle asapdiscovery/asapdiscovery#472

Closed

drop in redundant network fn

372d219

add tests

a1cd502

JenkeScheen changed the title ~~[WIP] add function to create a minimal network with redundancy for all nodes~~ add function to create a minimal network with redundancy for all nodes Sep 20, 2023

mikemhenry requested review from dwhswenson, richardjgowers and IAlibay September 20, 2023 16:00

dwhswenson requested changes Sep 20, 2023

View reviewed changes

Merge branch 'main' into main

a2dd148

dwhswenson reviewed Sep 20, 2023

View reviewed changes

openfe/setup/ligand_network_planning.py Outdated Show resolved Hide resolved

JenkeScheen and others added 11 commits September 21, 2023 09:49

more descriptive test fixture

7863505

set 'test_' for actual test function..

8f92cde

Merge branch 'main' of https://github.com/JenkeScheen/openfe_redundan…

a106976

…t_network

add test for number of edges in redundant networks

532b910

add mst_num = 2 by default

8caec4f

fix function names in redundant ntwk tests

229537a

add mst_num to default test

8457a3d

test that redundant(n=1) is same as spanning

a0bf736

pep8 shenanigans

f0fbdcc

redundant network into original MST test scope

a494fdf

JenkeScheen added 3 commits September 21, 2023 10:41

revert scope back to session so we can pull data

548568c

test fix (had wrong network refs)

deac790

more pep8

68a46a1

mikemhenry requested a review from dwhswenson September 21, 2023 16:13

simplify test

36a9f39

JenkeScheen and others added 2 commits September 25, 2023 09:25

rename duped fn name

0a97327

Merge branch 'main' into main

0334030

richardjgowers self-assigned this Oct 2, 2023

Merge branch 'main' into main

578b7a1

richardjgowers approved these changes Oct 10, 2023

View reviewed changes

Update ligand_network_planning.py

633837f

richardjgowers merged commit dc666ea into OpenFreeEnergy:main Oct 10, 2023
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add function to create a minimal network with redundancy for all nodes #559

add function to create a minimal network with redundancy for all nodes #559

JenkeScheen commented Sep 15, 2023 •

edited

Loading

richardjgowers commented Sep 15, 2023

codecov bot commented Sep 15, 2023 •

edited

Loading

JenkeScheen commented Sep 18, 2023

JenkeScheen commented Sep 18, 2023

richardjgowers commented Sep 18, 2023

JenkeScheen commented Sep 18, 2023

dwhswenson left a comment

pep8speaks commented Sep 20, 2023 •

edited

Loading

JenkeScheen commented Sep 21, 2023

mikemhenry commented Sep 21, 2023

JenkeScheen commented Sep 22, 2023

mikemhenry commented Sep 22, 2023

mikemhenry commented Sep 26, 2023 •

edited

Loading

mikemhenry commented Oct 5, 2023

IAlibay commented Oct 5, 2023

IAlibay commented Oct 5, 2023

add function to create a minimal network with redundancy for all nodes #559

add function to create a minimal network with redundancy for all nodes #559

Conversation

JenkeScheen commented Sep 15, 2023 • edited Loading

Developers certificate of origin

richardjgowers commented Sep 15, 2023

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

JenkeScheen commented Sep 18, 2023

JenkeScheen commented Sep 18, 2023

richardjgowers commented Sep 18, 2023

JenkeScheen commented Sep 18, 2023

dwhswenson left a comment

Choose a reason for hiding this comment

pep8speaks commented Sep 20, 2023 • edited Loading

Comment last updated at 2023-10-10 08:28:18 UTC

JenkeScheen commented Sep 21, 2023

mikemhenry commented Sep 21, 2023

JenkeScheen commented Sep 22, 2023

mikemhenry commented Sep 22, 2023

mikemhenry commented Sep 26, 2023 • edited Loading

mikemhenry commented Oct 5, 2023

IAlibay commented Oct 5, 2023

IAlibay commented Oct 5, 2023

JenkeScheen commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading

pep8speaks commented Sep 20, 2023 •

edited

Loading

mikemhenry commented Sep 26, 2023 •

edited

Loading