Cube embedding#280
Open
mahdiehmalekian wants to merge 1 commit into
Open
Conversation
mahdiehmalekian
force-pushed
the
cube_embedding
branch
from
6d0ebcc to
2a5604e
Compare
SebastianGitt
left a comment
SebastianGitt
left a comment
There was a problem hiding this comment.
Looking good, just did a first pass highlighting mostly minor issues before I take a deeper look. I did find what may be a couple of small bugs.
| _fields = ("u", "w", "k", "j") | ||
|
|
||
| def to_tuple(self) -> tuple[int]: | ||
| """Rerurns the tuple corresponding to the coordinate.""" |
There was a problem hiding this comment.
Suggested change
| """Rerurns the tuple corresponding to the coordinate.""" | |
| """Returns the tuple corresponding to the coordinate.""" |
| _fields = ("u", "w", "j") | ||
|
|
||
| def to_tuple(self) -> tuple[int]: | ||
| """Rerurns the tuple corresponding to the coordinate.""" |
There was a problem hiding this comment.
Suggested change
| """Rerurns the tuple corresponding to the coordinate.""" | |
| """Returns the tuple corresponding to the coordinate.""" |
|
|
||
| Provides convenient representations and helpers for embedding-related algorithms. | ||
| Example: | ||
| >>> from burnaby.lattice_embedding.auxiliary_coordinates import UWKJ |
There was a problem hiding this comment.
Suggested change
| >>> from burnaby.lattice_embedding.auxiliary_coordinates import UWKJ | |
| >>> from minorminer._lattice_utils.auxiliary_coordinates import UWKJ |
| It captures the full coordinate of a Zephyr node. | ||
|
|
||
| Example: | ||
| >>> from burnaby.lattice_embedding.auxiliary_coordinates import UWKJZ |
There was a problem hiding this comment.
Suggested change
| >>> from burnaby.lattice_embedding.auxiliary_coordinates import UWKJZ | |
| >>> from minorminer._lattice_utils.auxiliary_coordinates import UWKJZ |
|
|
||
| # To make the node label consistent with ``G``'s | ||
| # (i.e. "int" or "coordinates") | ||
| if label == "coordinates": |
There was a problem hiding this comment.
should this be "coordinate" instead of "coordinates"? If so, should add a test using this branch since it wasn't caught
| if isinstance(val, int): | ||
| copied_dict[var] = val | ||
| elif isinstance(val, list): | ||
| copied_dict[var] = [k for k in val] |
There was a problem hiding this comment.
This is a shallow copy but the docstring says it returns a deep copy
| Returns: | ||
| dict[Edge, int]: The dictionary after removing all items with value zero. | ||
| """ | ||
| return {chain: chain_freq for chain, chain_freq in chain_freq.items() if chain_freq != 0} |
There was a problem hiding this comment.
chain_freq is used for both the dictionary and the dictionary values, consider renaming one of them to make it more clear
| from typing import Generator, NamedTuple | ||
|
|
||
| from minorminer.utils.zephyr.node_edge import Edge, NodeKind | ||
|
|
There was a problem hiding this comment.
inconsistent spacing here and elsewhere, I recommend running a code formatter like Black if you haven't already
| """ | ||
| A helper to represent some information of a path of chains of node indices (within a :class:`QuoTile`). | ||
| edge (Edge): Edge of node indices represnting the index-chain | ||
| pos (tuple[int, int]): pair of positions, the i-th element representing the number of occurence of i-th element of the index-chain when visiting the index-chain |
There was a problem hiding this comment.
should break this up over two lines so it doesn't exceed 100 characters (here and elsewhere)
| connectivity checks based on z-coupling. | ||
| 4. A lattice survey module that provides utilities for embedding lattices | ||
| on partially-yielded Zephyr graphs. | ||
| 5. A qoutient tile object to facilitate embedding lattice-type graphs on Zephyr. |
There was a problem hiding this comment.
Suggested change
| 5. A qoutient tile object to facilitate embedding lattice-type graphs on Zephyr. | |
| 5. A quotient tile object to facilitate embedding lattice-type graphs on Zephyr. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains the cube embedder together with the needed tools.
cube_embeddingcontainsperfect_cube_embedding.pywhich has the cube embedder function for graphs with Zephyr topology.The rest of
cube_embeddingfolder contains the necessary tools for performing the algorithm._lattice_utilscontains some tools that can be used for finding the embedding of a variety of lattices (2d or 3d) on Zephyr. These tools are also used incube_embedding.