Lazy succession diagram

[daemontus]

This PR is a substantial refactor of the SuccessionDiagram class which makes it (hopefully) simper and more modular while also more "correct". A rough overview of changes (with rationale) follows.

First, the list of "general" changes:

• Benchmark-related code moved to the benchmark directory. The reason is that adding bench_* scripts to the root folder makes it very cluttered. Unfortunately, this means that you'll have to run pip install . before running benchmarks (see benchmark/README.md).
• Minor changes to pyproject.toml to make nfvsmotifs install-able using pip.
• Refactor the CI script to separate individual tasks into jobs: this should be more flexible when trying to prevent timeouts.
• Each benchmark can now return a list of comma separated values and the benchmark runner will add these to the final aggregate table. In the previous PR, this was handled by a separate script, but it was very error prone. Having it in the benchmark runner directly means we can output almost anything without changing the runner or adding special aggregation logic.
• Test networks 122 and 144 are "disabled" because they have fundamental issues (too many min. trap spaces) that we'd have to overcome to be able to obtain any useful results for them.
• In space_utils, we can now compute an integer unique key for any space, assuming the underlying BooleanNetwork is available. This is used internally by SuccessionDiagram to identify duplicate nodes, but can be also used as a deterministic key for sorting a list of arbitrary spaces.
• In motif_avoidant, the computation of the retained set is refactored into a separate function, because now it is actually used in multiple places (and it makes it easier to refactor/change in the future in general).

Second, the list of changes to SuccessionDiagram specifically:

• Non-trivial algorithms are now delegated to the _sd_algorithms module (see below). The SuccessionDiagram class just calls the functions from this module. The reason is that the SuccessionDiagram class needs to support a lot of non-trivial algorithms and it makes it easier to update/validate these algorithms in isolation.
• Properties expanded, attr_expanded and attractors of the succession diagram are now just properties of the individual nodes of the networkx graph. This slightly complicates typing but is more "modular" in the long-term.
• Compared to the previous PR, nodes are not explicitly marked as "stub". A "stub" is simply any node that is not expanded. The reason for this (compared to the previous PR) is that I realized that the only reason for having both expanded and stub flags was the presence of a "default" expansion algorithm (i.e. we needed a mechanism that would tell the default algorithm to skip some nodes). Now that the SD does not (internally) care about expansion algorithms at all, expanded/non-expanded is the only distinction that we need.
• There is a bunch of new "getters" for various properties/iterators (e.g. find node id based on its trap space, iterate over all stub nodes, etc.).
• We can now test whether an SD is a subgraph of another SD, or if two SDs are isomorphic. This is very useful for testing. Note that this takes into account the trap space of each node, but not the stable motifs on the edges. This is because in some cases, a single node (after percolation) can be reached using multiple edges and we store just one stable motif in such case. This is not ideal (we are losing some information), but for now it does not seem to be a big problem.
• In the _sd_algorithms` module, we provide methods for:
  • Computing the attractor "seed" states for any SD node (previously expand_attractors).
  • Expanding the SD fully using BFS or DFS (if SD can be fully expanded, the result is the same. It only differs if we set a limit on SD size/depth).
  • Expanding the SD partially, such that each minimal trap is expanded. In some sense, this is the "minimum viable" succession diagram.
  • Expanding the SD partially based on a target space, such that any space that intersects with this target is expanded, as long as it is not a proper subspace of target.
  • Expanding the SD partially such that a node is expanded if there are some attractor candidate states in it. In other words, the SD is expanded in a maximal way that still helps with attractor search. Expanding any further nodes is not relevant for NFVS attractor search method.

Furthermore, tests were expanded to cover more cases (we're at 92% coverage compared to 89% on main while adding a bunch of new code). I also tried to make extra sure that the results are deterministic (sorting nodes by node id, spaces by unique key, or explicitly documenting outputs where the order is not guaranteed).

Obviously, most of these are breaking changes but I'll update other active branches ones this is merged.

Performance: I'm still waiting for all performance numbers, but overall what I've seen so far is comparable to the previous PR. The main difference is that this PR does not use symbolic algorithms at all, hence cases where attractor detection which benefited from them might be slower than the previous PR. But this does not concern the speed of SD expansion. I'll experiment a bit more with symbolic attractor detection and attractor detection in general and put these into a separate PR.

[github-actions]

Coverage Report

File	Stmts	Miss	Cover	Missing
nfvsmotifs
SuccessionDiagram.py	148	5	97%	6–7, 189, 296, 383
interaction_graph_utils.py	142	8	94%	6–9, 57, 70, 95–96
motif_avoidant.py	116	4	97%	25, 58, 72, 110
petri_net_translation.py	84	6	93%	23–24, 52, 63–64, 94
pyeda_utils.py	95	34	64%	12, 56–66, 90, 95, 98–112, 140–144
space_utils.py	118	14	88%	15–16, 36–43, 52, 198, 213, 270
state_utils.py	68	12	82%	15, 55–66, 98, 105, 114
terminal_restriction_space.py	44	3	93%	6–7, 80
trappist_core.py	186	22	88%	10–11, 39, 41, 81, 122, 182, 184, 186, 221–223, 249, 260–261, 306, 308, 338, 378, 380, 411, 440
nfvsmotifs/FVSpython3
FVS.py	48	10	79%	90–91, 97, 133, 183–189
FVS_localsearch_10_python.py	90	1	99%	179
nfvsmotifs/_sd_algorithms
compute_attractor_seeds.py	26	1	96%	6
expand_bfs.py	28	1	96%	6
expand_dfs.py	30	1	97%	6
expand_minimal_spaces.py	35	1	97%	6
TOTAL	1355	123	91%

Tests	Skipped	Failures	Errors	Time
289	0 💤	0 ❌	0 🔥	2m 43s ⏱️

[daemontus]

TODO for this PR: Add expansion algorithm that can partially expand the succession diagram towards a specific target subspace (i.e. not expand things that contradict this space).

[github-actions]

Coverage Report

File	Stmts	Miss	Cover	Missing
nfvsmotifs
SuccessionDiagram.py	154	4	97%	6–7, 191, 411
interaction_graph_utils.py	142	8	94%	6–9, 57, 70, 95–96
motif_avoidant.py	164	6	96%	24–25, 125, 142, 179, 303
petri_net_translation.py	84	6	93%	23–24, 52, 63–64, 94
pyeda_utils.py	95	34	64%	12, 56–66, 90, 95, 98–112, 140–144
space_utils.py	118	5	96%	15–16, 198, 213, 270
state_utils.py	68	12	82%	15, 55–66, 98, 105, 114
terminal_restriction_space.py	44	3	93%	6–7, 80
trappist_core.py	186	22	88%	10–11, 39, 41, 81, 122, 182, 184, 186, 221–223, 249, 260–261, 306, 308, 338, 378, 380, 411, 440
nfvsmotifs/FVSpython3
FVS.py	48	10	79%	90–91, 97, 133, 183–189
FVS_localsearch_10_python.py	90	1	99%	179
nfvsmotifs/_sd_algorithms
compute_attractor_seeds.py	28	1	96%	6
expand_attractor_seeds.py	51	4	92%	6, 86–89
expand_bfs.py	28	1	96%	6
expand_dfs.py	30	1	97%	6
expand_minimal_spaces.py	37	1	97%	6
expand_to_target.py	30	3	90%	6, 34, 39
TOTAL	1494	122	92%

Tests	Skipped	Failures	Errors	Time
353	0 💤	0 ❌	0 🔥	3m 21s ⏱️

[daemontus]

I think the whole thing is ready for review now :) As mentioned, things around attractor detection need to be benchmarked/optimized a bit more, but at least all the functionality we have on main should now be there.

[jcrozum]

this is OK for now, but these kind of functions make me think that we might want to consider a Space class that implements __hash__. To be clear, I'm not saying that's necessarily the right thing to do, just that we should think about it.

[daemontus]

Oh yeah, absolutely!

[jcrozum]

Why is this a separate function and not just an option integrated into expand_bfs (and expand_dfs)?

[daemontus]

My original intuition was that this will be used by the control algorithm and hence there may be features/options that I am not aware of right now but may be incompatible with the "generic" DFS/BFS algorithms.

For example, that you might want to modify the search procedure in some way to "optimise" the SD for control even further. E.g. by picking a specific path to target instead of the "default" path chosen by BFS/DFS.

But I agree that in principle the algorithms could be merged.

[jcrozum]

I just finished going over everything. I have only a few minor issues to raise:

1. As I commented above, I don't understand why expand_to_target is separate from expand_bfs and expand_dfs. If there's no particular reason, I think it would be good to combine these for ease of maintenance. That being said, I don't think this is a big deal and combining the functions can always be done when the need actually arises.
2. Some more tests of the partial expansion would be nice to have, but the control stuff will at least partially test these, so I don't think this should stop us from merging.
3. We should consider making a Space class sometime. Again, this can wait.

Overall, it looks good to me, and I'd be happy to merge it.

[jcrozum]

I'm just going to merge this and get to work resolving conflicts with the control branch.