-
Notifications
You must be signed in to change notification settings - Fork 45
Zephyr utils #261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Zephyr utils #261
Conversation
thisac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check indent
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.