Put linking and non-linking nonterminals in (hopefully) faster order in multi_solve #186
Conversation
davidweichiang
changed the title
davidweichiang
changed the title
|
Closes #179. |
fggs/multi.py
Outdated
|
|
||
| for neighbor in nonterminal_graph.get(node, []): | ||
| if neighbor not in visited: | ||
| dfs(neighbor, iteration + 1) |
There was a problem hiding this comment.
You're computing the depth in the DFS tree, but the finishing times are supposed to be unique (if u != v then finish[u] != finish[v]). What you did might also work, but I'll have to think about it.
There was a problem hiding this comment.
Assuming that the DFS iterates over children left-to-right (so, A before D), I believe the depths and finishing times above are correct. And according to the current code, I think C and D would both be non-linking nonterminals. But according to the paper, just D would be a non-linking nonterminal.
There was a problem hiding this comment.
You're definitely right here. I'll make this change.
fggs/multi.py
Outdated
| #dfs on graph to get finish times | ||
| finish_times = {} | ||
| visited = set() | ||
| time = [0] #this must be a list so it is mutable and will persist over multiple calls to dfs |
There was a problem hiding this comment.
You can just make this an int, i.e., time = 0, then inside dfs, write nonlocal time, then to increment time, write time += 1
test/test_multi.py
Outdated
| @@ -81,6 +82,13 @@ def test_multi_solve(self): | |||
| x_sparse = multi_solve(a_sparse, b_sparse, transpose=True) | |||
| x_dense = torch.linalg.solve(torch.eye(*a_dense.shape)-a_dense.T, b_dense) | |||
| self.assertTrue(self.compare_vectors(x_sparse, x_dense), (x_sparse, x_dense)) | |||
|
|
|||
| ### WIP ### | |||
There was a problem hiding this comment.
Does this still need work?
There was a problem hiding this comment.
This test is pretty basic and I think there should probably be some more testing. There isn't currently a great fgg in the test directory to test this function with. Do you think I should make a new fgg to test this with?
There was a problem hiding this comment.
Is example12p.json not good? I think it would be a lot of work for you to make a whole new FGG, but you could. Or we can build one from a PERPL program.
There was a problem hiding this comment.
example12.json and cycle.json both have a nonlinking nonterminal, but they are already at the end of a.shapes[0].keys() by default, so _order_nonterminals doesn't actually change anything when run on them.
I just added advanced_cycle.json which doesn't have this issue.
test/test_multi.py
Outdated
| @@ -1,7 +1,10 @@ | |||
| from fggs.multi import * | |||
| import fggs.multi as multi_module | |||
There was a problem hiding this comment.
Please import as multi or if not possible, then as fggs.multi
test/advanced_cycle.json
Outdated
There was a problem hiding this comment.
Could we call it double_cycle.json instead?
No description provided.