reconciling #71 and #73 for #69 #74
Conversation
|
Note that most of the changes in this PR are cosmetic (type-checking & linting stuff). The functional changes are in the two new files ( |
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@daemontus I'm requesting your review just to make sure I didn't break anything you added in #71. |
nfvsmotifs/control.py
Outdated
| target=target, | ||
| ) | ||
|
|
||
| for s in cast(list[int], succession_diagram.G.nodes()): |
There was a problem hiding this comment.
This could be succession_diagram.node_ids() and succession_diagram.node_space(s). It's not faster, but it ensures the casts are limited to the SuccessionDiagram class (it should make it easier to refactor SuccessionDiagram if we need to do that at some point).
nfvsmotifs/control.py
Outdated
| ) | ||
| is_last_needed = set(target) <= set(fixed_vars) | ||
|
|
||
| if not is_consistent or not is_last_needed: |
There was a problem hiding this comment.
I'm not quite sure, but wouldn't this be equivalent to not space_utils.is_subspace(fixed_vars, target)?
nfvsmotifs/control.py
Outdated
| ), | ||
| ): | ||
| succession = [ | ||
| cast(dict[str, int], succession_diagram.G.edges[x, y]["motif"]) |
There was a problem hiding this comment.
Maybe SuccessionDiagram.edge_stable_motif(x,y)?
|
I think everything looks good, I just made a few comments in places where I believe we can make a better use of the existing utility functions :) Feel free to merge once these are fixed or if you believe these are not justified. |
|
These are good changes. I'll implement them and make sure it all still works. |
| assert all([controls_are_equal(a, b) for a, b in zip(cs, drivers)]) | ||
|
|
||
| target_succession = [ | ||
| {"E": 1}, |
There was a problem hiding this comment.
Can you elaborate a bit on why E=1 should be a separate succession? Should it always be a separate succession if a constant node has to be fixed into its opposite state?
There was a problem hiding this comment.
In this test, target_succession is the sequence of nested trap spaces that I want to drive the system toward. In other words, it can be arbitrary. This part of the test is highlighting that it still works even when the succession you're trying to reach is not part of the natural dynamics.
| A, S | B | ||
| B, A | ||
| C, A | D | ||
| D, C |
There was a problem hiding this comment.
How about changing this to "D, C | E" so that there is a possible external control? What I'm not sure about is how the successions should look like in this case.
There was a problem hiding this comment.
This one already has an external control: {"A": 1} and {"B": 1} are external controls for {"C": 1, "D": 1}
There was a problem hiding this comment.
Although, now that I think about it, I'm not sure how the code would handle the possibility of overriding constant nodes. I suspect it would not find these cases.
This supersedes #73; we should merge this PR and close #73 when we are ready.