Conversation
thisac
left a comment
There was a problem hiding this comment.
Thanks for the hard work @mahdiehmalekian. Had a first pass through node_edge. Will come back to it again, and the other files, in a future review soon.
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
thisac
left a comment
There was a problem hiding this comment.
Had another review round. Haven't checked the tests yet (will do that next).
A couple of overall comments and suggestions:
qfloor.pycontains a couple of methods without or with incomplete docstrings.- Code in docstrings should have double back-ticks (
``), and wherever possible, it's nice (but not mandatory) to have cross-references to classes and methods, etc. See here. - A couple of places I'd recommend adding a new line after returns and around for-loop-blocks, try-excepts, after raises and after returns, to make it a bit more legible.
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
Co-authored-by: Radomir Stevanovic <radomir.stevanovic@gmail.com>
randomir review implementation Co-authored-by: Radomir Stevanovic <radomir.stevanovic@gmail.com>
thisac review code suggestions Co-authored-by: Theodor Isacsson <theodor@isacsson.ca>
…anch with testing derived from TilingComposite
Co-authored-by: Radomir Stevanovic <radomir.stevanovic@gmail.com>
### New Features - Allow lattice type and lattice dimensions to be passed as options for `find_sublattice_embeddings()`. See [\dwavesystems#266](dwavesystems#266). ### Bug Fixes - Fix `find_sublattice_embeddings()` to not produce non-disjoint embeddings when `use_tile_embedding=True` is set. See [\dwavesystems#265](dwavesystems#265).
) * Added support for colorings in Glasgow subgraph solver * updated Glasgow to latest version (with patches for c++17 support) * new parameters for subgraph.find_subgraph * as_embedding=True will produce a dict with tuple values * node_labels to be respected by GSS, node labels must match * edge_labels as with node_labels; directed edge labels match
* removed kwargs handling in favor for explicit arguments (issue dwavesystems#255) * added injectivity parameter to support noninjective and locally-injective homomorphisms * fix isolated nodes issue dwavesystems#254 and add injectivity options * support random seeds (issue dwavesystems#243)
* added ability to interrupt find_subgraph via ctrl-c
randomir
left a comment
There was a problem hiding this comment.
Very well written code and super useful utils! Thank you, @mahdiehmalekian!
My feedback is mostly about code/docs style (for consistency with Ocean) and coding best practices.
In terms of interface/API, I would simplify ZNode by defining neighbor generators only, also removing membership test functions.
| xyks = [(0, 1), (1, 0, None), (12, -3)] | ||
| ccoords = [CartesianCoord(*xyk) for xyk in xyks] | ||
| for ccoord in ccoords: | ||
| cartesian_to_zephyr(ccoord=ccoord) |
There was a problem hiding this comment.
I would fold the two *_runs tests with the parameterized ones, but if you want to keep them as smoke tests, I recommend at least testing the returned type:
| cartesian_to_zephyr(ccoord=ccoord) | |
| self.assertIsInstance(cartesian_to_zephyr(ccoord=ccoord), ZephyrCoord) |
| uwkjzs = [(0, 2, 4, 1, 5), (1, 3, 3, 0, 0), (1, 2, None, 1, 5)] | ||
| zcoords = [ZephyrCoord(*uwkjz) for uwkjz in uwkjzs] | ||
| for zcoord in zcoords: | ||
| zephyr_to_cartesian(zcoord=zcoord) |
There was a problem hiding this comment.
Same comment as above:
| zephyr_to_cartesian(zcoord=zcoord) | |
| self.assertIsInstance(zephyr_to_cartesian(zcoord=zcoord), CartesianCoord) |
| Returns: | ||
| PlaneShift: The displacement in CartesianCoord by self followed by other. | ||
| """ | ||
| if type(self) != type(other): |
There was a problem hiding this comment.
By using isinstance, you support PlaneShift subclasses for other:
| if type(self) != type(other): | |
| if not isinstance(other, PlaneShift): |
| for nbr in self.internal_neighbors_generator(where=where): | ||
| if other == nbr: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
This, and the similar 3 other functions feel redundant as they can be replaced with:
| for nbr in self.internal_neighbors_generator(where=where): | |
| if other == nbr: | |
| return True | |
| return False | |
| return other in self.internal_neighbors_generator(where=where) |
| self, | ||
| nbr_kind: EdgeKind | Iterable[EdgeKind] | None = None, | ||
| where: Callable[[CartesianCoord | ZephyrCoord], bool] = lambda coord: True, | ||
| ) -> list[ZEdge]: |
There was a problem hiding this comment.
Why not make this function a generator?
| return self._ccoord == other._ccoord | ||
|
|
||
|
|
||
| def __gt__(self, other: ZNode) -> bool: |
There was a problem hiding this comment.
Instead of manually defining all comparison methods, use @functools.total_ordering.
| ] | ||
| ) | ||
| def test_degree(self, xy, m, nbr_kind, a, b) -> None: | ||
| for t in [None, 1, 4, 6]: |
coordinate_systems.py: Contains namedtuples for Zephyr and Cartesian coordinates and the conversion functions between the twoplane_shift.py: Contains the class for displacement of nodes in the Cartesian plane.node_edge.py: Contains the class for edge, edge of a Zephyr graph (which checks whether the edge exists in a perfect yield Zephyr graph), and the node of a Zephyr graphqfloor.py: Contains the class for working with a tile (subset of nodes) in quotient Zephyr. Also contains the class for putting the tiles together in a number of rows and columnssurvey.py: Contains the class for taking a survey of a Zephyr graph compared to a perfect-yield Zephyr graph on the same grid and tile sizeThe test suite contains the tests for these modules.