jcrozum / biobalm

The biologist's Boolean attractor landscape mapper, building Waddington landscapes from Boolean networks.
https://jcrozum.github.io/biobalm/
MIT License
2 stars 0 forks source link

Attractor detection refactoring #116

Closed daemontus closed 7 months ago

daemontus commented 7 months ago

This PR proposes a major refactoring/update of the attractor detection mechanism to make it more practical on large networks.

I have slightly updated the vocabulary in the code:

Highlights:

Performance results for "seed" state detection:

Full attractor results (first is the new version, second is the old version, the random pass on the old version failed before the end due to unrelated reasons, but I think the picture is clear) full-attr.xlsx. There is one notable regression in the BBM dataset (ID195): here, the attractor search is very very simple and it actually isn't worth it to even compute the percolated network for each trap space, that's why it is taking longer now. But the actually hard instances like 002 or 211 do seem to benefit quite a bit, not just the random models.

Other relevant changes:

TODOs:

github-actions[bot] commented 7 months ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
biobalm
   _pint_reachability.py615018%24, 40–54, 69–93, 101–146
   control.py1141488%107, 119, 125, 129, 134, 143–159, 477, 480, 493
   interaction_graph_utils.py38489%11–13, 151–152
   petri_net_translation.py1491193%22–26, 79, 136, 308–309, 333–334, 343, 452
   space_utils.py1321688%26–28, 104–110, 133–139, 414, 462
   succession_diagram.py3816683%6, 118, 207–212, 225, 272–279, 383–390, 407–408, 418, 424, 540, 627–633, 749, 752, 870–888, 920, 930, 933, 970, 973, 980, 1031, 1045, 1125, 1308, 1319, 1327, 1355, 1370, 1382, 1387, 1393
   symbolic_utils.py32584%10, 39–44, 100, 128
   trappist_core.py1832785%14–18, 55, 57, 92, 215, 217, 219, 247–250, 254–256, 276–282, 340, 342, 372, 420, 422
biobalm/_sd_algorithms
   expand_attractor_seeds.py60788%6, 28, 42, 109–114, 119
   expand_bfs.py28196%6
   expand_dfs.py30197%6
   expand_minimal_spaces.py37295%6, 31
   expand_source_SCCs.py122497%14–16, 88, 133
   expand_to_target.py31390%6, 38, 43
biobalm/_sd_attractors
   attractor_candidates.py2638966%13–15, 26–27, 93, 101, 107–108, 130, 152, 187, 193–204, 223, 239–316, 321, 325, 331, 337, 348, 372, 377, 381, 387, 389–427, 500, 571–572, 673
   attractor_symbolic.py1141686%6–7, 75, 88–92, 103, 112, 144, 179, 191–193, 202, 230, 236
TOTAL186031683% 

Tests Skipped Failures Errors Time
357 0 :zzz: 0 :x: 0 :fire: 44.766s :stopwatch:
jcrozum commented 7 months ago

@daemontus any thoughts on how to reconcile this with #115? I'm thinking merge this first, then revert #115 to before the package rename and merge that, then make an entirely new pr just for the renaming. Not sure if there's a better way.

daemontus commented 7 months ago

I just tried to rebase it locally from #115 and it seems that we can first do #115 and then I'll just update this PR with the new name. Interestingly, other than the the newly created files/functions, most of the changes merged without conflicts... so it should be an easy fix :)

daemontus commented 7 months ago

This should now be fully rebased with the latest name change. (bcc: @jcrozum)

daemontus commented 7 months ago

@jcrozum I think I resolved the issue with the "constant configuration values": SuccessionDiagram now has a configuration dictionary that stores all of these and you can pass it as a constructor parameter. As such, if you serialize the diagram, the "configuration" is preserved. Furthermore, these are no longer global module "constants" so we don't have to worry about users needing to access/modify them.

Please have a quick look if that looks reasonable to you, and if yes, feel free to merge the PR.

jcrozum commented 7 months ago

Looks good, I'll merge it.