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

simplify vertex cover code for unyielded edges. Add support for zephyr or zephyr #297

Merged
merged 14 commits into from
Nov 26, 2024

Conversation

jackraymond
Copy link
Contributor

Two enhancements:
Add support for zephyr lattices on zephyr solvers (Advantage2)
Improve handling of unyielded edges, improving performance for lower yield processors using dwave-ocean-sdk and networkx algorithms.

hybrid/decomposers.py Outdated Show resolved Hide resolved
Copy link
Member

@randomir randomir left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits below.

hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
@jackraymond
Copy link
Contributor Author

I think I have implemented all the review suggestions.
Per code suggestion: I bypassed the use of exception catching in the _good_cover routine by calculating the tree width requirement explicitly - there is a bit of untidy (practically irrelevant) redundancy this way but still a big improvement.

hybrid/decomposers.py Outdated Show resolved Hide resolved
hybrid/decomposers.py Outdated Show resolved Hide resolved
jackraymond and others added 3 commits November 20, 2024 06:26
Co-authored-by: Radomir Stevanovic <[email protected]>
Co-authored-by: Radomir Stevanovic <[email protected]>
… on defective Z12s (all branches). Necessary future work - write tests for _good_cover and related defect handling since tests are mostly full yield and are missing errors.
@jackraymond
Copy link
Contributor Author

I neglected to test all of the _good_cover code, this has now been done. Note for next pull request - the tests are currently insufficient and don't execute this function (the full workflow tests work with defect free lattices), need to add new unit tests.
For now I have manually tested all branches.


import dimod
from dimod.traversal import connected_components
import dwave_networkx as dnx
import dwave.preprocessing
from dwave.samplers import SteepestDescentSolver, TreeDecompositionSolver
Copy link
Member

Choose a reason for hiding this comment

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

@jackraymond, some tests are failing because dwave.samplers is not generally available for all versions of python/dimod.

For details see #284, but TL;DR: we need to drop support for dimod<0.12 in order to switch to dwave-samplers/dwave.samplers.

Also, to even get access to Orang / TreeDecompositionSolver (only available as part of dwave-samplers) -- which is a pretty compelling reason.

I'll do that in a separate PR, and then we can rebase this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lazy with my full environment testing!

Copy link
Member

Choose a reason for hiding this comment

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

@jackraymond, with #299 merged, your code should now work after you rebase to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@randomir randomir merged commit c9741e8 into dwavesystems:master Nov 26, 2024
29 checks passed
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 79.48718% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.87%. Comparing base (9ee89a3) to head (8ae64e1).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
hybrid/decomposers.py 82.85% 6 Missing ⚠️
hybrid/reference/lattice_lnls.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
- Coverage   92.09%   91.87%   -0.23%     
==========================================
  Files          18       18              
  Lines        2252     2264      +12     
==========================================
+ Hits         2074     2080       +6     
- Misses        178      184       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@randomir randomir mentioned this pull request Dec 3, 2024
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