Skip to content

Add Grid for surveying Zephyr topologies - #291

Open
mahdiehmalekian wants to merge 5 commits into
dwavesystems:mainfrom
mahdiehmalekian:grid
Open

mahdiehmalekian wants to merge 5 commits into
dwavesystems:mainfrom
mahdiehmalekian:grid

Conversation

@mahdiehmalekian

Copy link
Copy Markdown
Contributor

Adds minorminer.utils._clique.grid, another internal piece of the Zephyr clique embedder, after el_geometry (#289) and maximum_bipartite_matching (#290).

Grid surveys a possibly-faulty Zephyr topology once, at construction, into tables the clique embedder then queries directly. Tests are in tests/utils/clique/test_grid.py.

AI use: I used Claude to clean up and edit documentation of the grid.py and to generate the test_grid.py. Checked and edited myself.

Note to reviewers: This is part of a larger Zephyr clique embedder effort and subsequent clique-embedder PRs depend on this PR and can only follow once this merges. Would appreciate a speedy review.

@SebastianGitt SebastianGitt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I think many of the methods would be more readable if they had more descriptive variable names.

Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread tests/utils/clique/test_grid.py Outdated
Comment thread tests/utils/clique/test_grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated

@SebastianGitt SebastianGitt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took another pass with some higher-level observations and suggestions.

Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py
Comment thread minorminer/utils/_clique/grid.py
Comment thread minorminer/utils/_clique/grid.py
Comment thread minorminer/utils/_clique/grid.py Outdated

@thisac thisac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mahdiehmalekian. I'd like to do a more thorough review.

Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Comment thread minorminer/utils/_clique/grid.py Outdated
Build Grid entirely in __init__ (remove from_graph)
Bump dwave-graphs to >=1.2 to use zephyr_coordinates.zephyr_to_cartesian instead of a local conversion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants