Skip to content
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

TilingComposite support for Advantage processor #431

Merged
merged 10 commits into from
Nov 10, 2021

Conversation

jackraymond
Copy link
Contributor

The TilingComposite has been modified to allow tiling of Chimera structured problems over Pegasus as well as Chimera solvers.
This is an important use case for certain users, who wish to compare Chimera-structured (or chimera embeddable) problems between the DW2000Q and Advantage generation processors, at high sample density.
Some minor fixes were also introduced for Chimera case, catching some pathological behaviours.

The DWaveMockSampler has been modified to support Pegasus as well as Chimera structured solver emulation.

These changes allow straightforward generalization for the case of Zephyr topologies.
These changes do not resolve all known outstanding issues with TilingComposite e.g. #295 . This supports tiling of Chimera problems over Advantage (Pegasus-structured) processors, further changes are required to support tiling of Pegasus problems over Advantage (or DW2000Q) processors.

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

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

Some comments on the docstrings, I'll look at the code next

(Happy you're doing this; we'll finally be able to dump #294 :-))

dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
dwave/system/composites/tiling.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2021

Codecov Report

Merging #431 (d1e6399) into master (10acaa5) will decrease coverage by 3.25%.
The diff coverage is 88.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
- Coverage   90.85%   87.60%   -3.26%     
==========================================
  Files          22       22              
  Lines        1455     1492      +37     
==========================================
- Hits         1322     1307      -15     
- Misses        133      185      +52     
Impacted Files Coverage Δ
dwave/system/composites/tiling.py 91.75% <88.00%> (-4.31%) ⬇️
dwave/system/testing.py 96.36% <88.46%> (-2.58%) ⬇️
dwave/system/samplers/leap_hybrid_sampler.py 61.72% <0.00%> (-13.88%) ⬇️
dwave/system/samplers/clique.py 77.35% <0.00%> (-5.04%) ⬇️
dwave/system/samplers/dwave_sampler.py 84.47% <0.00%> (-3.11%) ⬇️
dwave/system/composites/embedding.py 95.85% <0.00%> (-1.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10acaa5...d1e6399. Read the comment docs.

Copy link
Member

@arcondello arcondello left a comment

Choose a reason for hiding this comment

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

Can we add a test for a pegasus processor with missing qubits?

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

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

Not due to this PR but I think tiling.py is missing an import warnings statement for

warnings.warn("Incomplete solver topology information."

@jackraymond
Copy link
Contributor Author

Testing both edge defects and node defects independently for the tiling composite (and other functions) is quite important so I added that functionality to MockDWaveSampler as well.
Other changes on November 8th are superficial - mostly PEP8 stuff, and doesn't impact tiling composite other than an expansion of the tests run.

@arcondello
Copy link
Member

@JoelPasvolsky, do you want to finish reviewing?

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

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

LGTM, gives good results for all the tests I tried.

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.

4 participants