Succession diagram storage #28
Conversation
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
nfvsmotifs/space_utils.py
Outdated
| @@ -76,18 +76,24 @@ def percolate_space(network: BooleanNetwork, space: dict[str, str]) -> tuple[dic | |||
| for var in network.variables(): | |||
| var_name = network.get_variable_name(var) | |||
| expression = aeon_to_pyeda(network.get_update_function(var)) | |||
|
|
|||
| # if the var is already constant, it doesn't count | |||
| if expression == PYEDA_TRUE or expression == PYEDA_FALSE: | |||
There was a problem hiding this comment.
Isn't this a problem for percolation of constants/inputs? Let's have x = true, but as the input space, I provide a dictionary where x does not have a fixed value. Then the value of x never makes it into the result space, even though the percolation should set it to true?
Edit: I'll push a test to demonstrate shortly :)
There was a problem hiding this comment.
Ok... I have pushed a test that demonstrates this and fails on my machine.
Now, it may very well be that I don't understand something about the semantics of percolate_space, which would then make this behaviour ok. If that's the case, please update the test to what you believe is the right behaviour for that input :)
There was a problem hiding this comment.
I think this is a matter of convention, but as you note, it has other implications. It is sometimes useful to distinguish between variables that become fixed as a result of the external override vs as a result of the natural dynamics. I changed it here because it was leading to some inconsistent results regarding driver nodes, etc., but after seeing your comment, I realize I that it would be beneficial to have a flag to determine which behavior is desired. I'll make an issue to track this, and we can discuss it further there if needed.
Edit: see #32
There was a problem hiding this comment.
I see. I don't really have a specific inclination towards either variant. I would just like to have it defined as exactly as possible, because when we just write "percolation", intuitively I see both interpretations as meaningful and don't know which one is the one we actually want.
| self.G.add_node(0, fixed_vars={}, depth=depth) | ||
| sd_nodes_at_last_depth = {0} | ||
|
|
||
| while (sd_nodes_at_last_depth and depth < MAX_DEPTH and self.G.number_of_nodes() < MAX_SIZE): |
There was a problem hiding this comment.
Off topic question from a (mostly) non-Python user: Are the parenthesis here idiomatic or not? In Rust, automatic format tools would remove them, because they don't serve any semantic purpose (and AFAIK they are redundant here as well, right?).
With that said, I like some extra clarifying parenthesis from time to time and I don't really think they should be removed here. I was just beaten into submission by Rust linters, so I'm asking about the consensus in other languages :D
There was a problem hiding this comment.
I think you're right; these should be removed. Usually they are only used if the test condition is so long you need to split it across multiple lines.
|
I have looked at everything :) Aside from the small regression in Once you fix the failing test, you can merge this, no need to re-review it. |
|
I have addressed the percolation convention, and updated some tests to account for the convention difference. In some cases I just switched the assertion to the new default convention, in other cases I test both conventions. |
|
Awesome! Thank you very much :) Now everything is clear to me. |
No description provided.