Refactoring to support multigraph backend #217
Conversation
|
This passes all tests and typechecker, and at least AllFeatures.ipynb still runs as it is supposed to. Should I go ahead and merge into master? Maybe worth getting one more person to have a look in case I seriously broke something, e.g. @dlyongemallo? The current status is multigraph support via |
|
Exciting to see that multigraph is finally done. I can help in integrating it with ZXLive after the PR is merged. |
|
Is there any way to break this down into smaller, logically independent pieces, or is everything tied together in such a way that it would be impossible to separate? Can you add tests, especially where the multigraph backend behaves differently than the existing simple-graph one (ideally testing both backends to show the difference)? This is especially useful for catching edge cases. |
|
re: breaking in to chunks: not really. I just wanted to flag this up with a few other people to do a rough sanity check before I merge. This is a major change, so if some stuff breaks, we can deal with it as and when we find it. re: multigraph tests, those should be written eventually, but I think it's a good idea to go ahead and merge sooner rather than later because this touches so many files. Maybe it's a good idea for the multigraph backend to issue some kind of warning to use at your own risk until it has been tested a bit more extensively. |
| else: | ||
| if g2.edge_type(e) != EdgeType.HADAMARD: # We take Hadamard edges to be the default | ||
| self.changed_edge_types[e] = g2.edge_type(e) | ||
| # for e in new_edges: |
There was a problem hiding this comment.
Should this just be deleted? I noticed that there are a couple of places in pyzx where blocks of code are commented out without explanation. If these should be kept, it would be helpful to readers to have a preceding comment along the lines of "Alternate implementation of ..." or "Uncomment this block if you want to...". Otherwise, for someone new to a codebase who encounters blocks of code which are commented out, it's just puzzling why they're there.
| for _ in range(e.s): yield (s, t, EdgeType.SIMPLE) | ||
| for _ in range(e.h): yield (s, t, EdgeType.HADAMARD) | ||
|
|
||
| # def edges_in_range(self, start, end, safe=False): |
| return (s,t) if s < t else (t,s) | ||
| def edge_set(self): | ||
| return set(self.edges()) | ||
| def edge_st(self, edge): |
There was a problem hiding this comment.
edge_set and edge_st are dangerously close to "minor typo leads to frustrating-to-find bug" territory, though fortunately they have different signatures.
I don't see any major issues with the changes, and rather than block this on a more in-depth review, I don't object to merging this now. (I have some minor issues with places where more type hints would be helpful, or where blocks of code are commented out without explanation, but the existing codebase is already like this, and these can be fixed en masse later.) One thing I would strongly urge though is to change the commit message to be something like "[BREAKING CHANGE] Refactor to support multigraph backend" (per my comment zxcalc/zxlive#229 (comment)). |
|
Cool, done. I added [BREAKING CHANGES] to the commit msg for the merge commit. |
I needed to do some non-trivial refactoring to get PyZX ready for multigraph support (cf. issue #123).
The multigraph backend is still not completely ready, but I think now might be a good time to commit my refactoring to minimise conflicts going forward, since this touches almost every file. The main issue is to revert
ETto being an abstract type representing the identity of an edge, rather than an alias toTuple[VT,VT], because the latter no longer refers to a unique edge in the graph.Major changes are:
ETis now an opaque type, not an alias forTuple[VT,VT]. If I have aneof typeET, I should use the relevant graph methods to extract it's source and target, rather than simply callinge[0]ande[1].add_edgeandadd_edge_listshould take arguments of typeTuple[VT,VT], notETadd_edgein GraphS is now always "smart", i.e. it will try to handle parallel edges or throw an error. The old "dumb" version, where parallel edges get silently overwritten is gone.add_edge_tablejust repeated callsadd_edge, so all the parallel edge logic has been shifted toadd_edgeupairdefined inBaseGraphthat returns a given pair of vertices as an "unordered pair", i.e. in a canonical order. This is whatGraphS.edgedoes, but we should no longer assume all backends will do this.Note that some temporary hacks to get H and W nodes to work with parallel edges have not survived the refactor. I think it would be better instead to get rid of these hacks and assume with a working with multigraphs whenever we need parallel edges and non-ZX nodes. Unfortunately, this might temporarily break some ZXlive stuff.