Feature expansion and bug-fix #537

AndyZzzZzzZzz · 2024-09-19T23:47:47Z

Feature Expansion:

Added an option to specify which sampler to use for MockDWaveSampler in testing.py.
If no sampler is specified, the script defaults to using SteepestDescentSampler.
Introduced an additional option to specify mocking_sampler_params if the user decides to use a different sampler.

Tests:

Added two new tests in test_mock_sampler.py:
1. 'test_custom_mock_sampler' to verify that the script successfully switches between samplers.
2. 'test_mocking_sampler_params' to ensure that the script correctly accepts and processes substitute mocking sampler parameters.

Bug Fixes:

Fixed the calculation of the number of qubits for Pegasus topology (missing property that triggers an error when running tests locally).
Changed mock_parameters, substitute_sampler, and substitute_kwargs from class attributes to instance attributes to ensure instance independence.
Created a local variable for substitute_kwarg in the sample function to ensure that each sample gets its own copy.

…s variable status

This reverts commit 89ade5f.

…f mock sampler has its own copies

arcondello · 2024-09-19T23:50:13Z

Hi @AndyZzzZzzZzz looks like you accidentally included some extra files, including your whole virtual environment, in the commit.

codecov · 2024-09-19T23:51:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.83%. Comparing base (60a4e4b) to head (a064358).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
- Coverage   89.66%   86.83%   -2.84%     
==========================================
  Files          24       24              
  Lines        1761     1777      +16     
==========================================
- Hits         1579     1543      -36     
- Misses        182      234      +52

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

kevinchern · 2024-09-19T23:55:43Z

please add me and @jackraymond to reviewer

kevinchern

Add a release note, see dimod readme example
tips for writing commits 😃
Main concern is sample method should not consume args for the substitute sampler.
Careful with tests due to exact solver.

.gitignore

dwave/system/testing.py

tests/test_mock_sampler.py

kevinchern · 2024-09-20T00:46:30Z

tests/test_mock_sampler.py

+        custom_sampler = CustomSampler()
+
+        # Create a simple BQM
+        bqm = dimod.BQM({'a': -1, 'b': -1}, {('a', 'b'): -1}, 0.0, vartype='SPIN')


At this problem size, the exact solver would be triggered. should override the default exact_solver_cutoff

Another option is to return two samples (num_reads=2) and check the second sample. This might be simpler.

kevinchern · 2024-09-20T00:47:21Z

tests/test_mock_sampler.py

+        # Check that the sample returned is as expected from the custom sampler
+        expected_sample = {'a': 1, 'b': 1}
+        self.assertEqual(ss.first.sample, expected_sample)
+        self.assertEqual(ss.first.energy, bqm.energy(expected_sample))


these assertions will pass because they coincide with exact solver's solution

It would be useful if the bqm is defined so that the unique solution to the bqm is a ground state. I am not sure this is true for your Hamiltonian. If greedy initial state is (-1,-1) then flipping either spin yields energy zero and steepest descent will return the answer, but it is not a ground state.
Furthermore, define your constant sampler to return an excited state, that way it is guaranteed to conflict with both ExactSolver and SteepestDescentSolver defaults.
I've recommended code changes to achieve these things.

kevinchern · 2024-09-20T00:48:58Z

tests/test_mock_sampler.py

+        self.assertEqual(ss.first.sample, expected_sample)
+        self.assertEqual(ss.first.energy, bqm.energy(expected_sample))
+
+    def test_mocking_sampler_params(self):


will review after the comment on whether sample should consume kwargs for the substitute sampler has been addressed

kevinchern · 2024-09-20T00:49:41Z

dwave/.DS_Store

.gitignore

dwave/system/testing.py

jackraymond · 2024-09-20T16:30:15Z

dwave/system/testing.py

@@ -366,7 +397,7 @@ def sample(self, bqm, **kwargs):
                          if pair[1]!=3],dtype=float),
                [pair[0] for pair in initial_state if pair[1]!=3])

-        ss = SteepestDescentSampler().sample(bqm, **substitute_kwargs)
+        ss = self.substitute_sampler.sample(bqm, **substitute_kwargs)


We need to document that for the first sample the substitution is ignored unless self.exact_solver_cutoff=0; i.e. for small problems the first sample is always a ground state. See comment in tests.

jackraymond

Looks good overall. One issue in the tests to address.

jackraymond · 2024-09-20T16:33:09Z

tests/test_mock_sampler.py

+        custom_sampler = CustomSampler()
+
+        # Create a simple BQM
+        bqm = dimod.BQM({'a': -1, 'b': -1}, {('a', 'b'): -1}, 0.0, vartype='SPIN')


Another option is to return two samples (num_reads=2) and check the second sample. This might be simpler.

jackraymond · 2024-09-20T16:36:08Z

tests/test_mock_sampler.py

+        # Check that the sample returned is as expected from the custom sampler
+        expected_sample = {'a': 1, 'b': 1}
+        self.assertEqual(ss.first.sample, expected_sample)
+        self.assertEqual(ss.first.energy, bqm.energy(expected_sample))


It would be useful if the bqm is defined so that the unique solution to the bqm is a ground state. I am not sure this is true for your Hamiltonian. If greedy initial state is (-1,-1) then flipping either spin yields energy zero and steepest descent will return the answer, but it is not a ground state.
Furthermore, define your constant sampler to return an excited state, that way it is guaranteed to conflict with both ExactSolver and SteepestDescentSolver defaults.
I've recommended code changes to achieve these things.

tests/test_mock_sampler.py

dwave/system/testing.py

Co-authored-by: Jack Raymond <[email protected]>

jackraymond · 2024-09-27T16:33:14Z

dwave/system/testing.py

+            `sample` method. This allows users to configure the substitute sampler 
+            with specific parameters like `num_reads`, `initial_state`, or other 
+            sampler-specific options. If not provided, an empty dictionary is used 
+            by default.

        exact_solver_cutoff (int, optional, default=:attr:`EXACT_SOLVER_CUTOFF_DEFAULT`):


I think, it would make sense that if substitute_sampler is provided then this setting is ignored and the substitute sampler is applied for all samples. We can add this clarification (afer code modification).

jackraymond · 2024-09-27T16:37:35Z

dwave/system/testing.py

    EXACT_SOLVER_CUTOFF_DEFAULT = 16

    def __init__(self,
                 nodelist=None, edgelist=None, properties=None,
                 broken_nodes=None, broken_edges=None,
                 topology_type=None, topology_shape=None,
                 parameter_warnings=True,
+                 substitute_sampler=None, 
+                 substitute_kwargs=None, 
                 exact_solver_cutoff=EXACT_SOLVER_CUTOFF_DEFAULT,


Change this to None. See also comment on docstrings

if substitute_sampler is None: substitute_sampler = SteepestDescentSampler() if exact_solver_cutoff is None: exact_solver_cutoff = EXACT_SOLVER_CUTOFF_DEFAULT elif exact_solver_cutoff is None: exact_solver_cutoff = 0 # Always use substitute_sampler, never ExactSolver()

jackraymond

Looks good to me.
Someone @kevin should check my minor changes to test_mock_sampler.py just pushed.

kevinchern

~~- Add a release note, see dimod readme example.~~ not used

Squash commits

LGTM otherwise

kevinchern · 2024-10-07T18:50:19Z

tests/test_mock_sampler.py

+        # use SteepestDescentSampler()
+        ss = sampler.sample(bqm, num_reads=2)
+        self.assertEqual(sampler.exact_solver_cutoff, 0)
+        self.assertEqual(ss.record.sample.shape, (1,2), 'Unique sample expected')


Suggested change

self.assertEqual(ss.record.sample.shape, (1,2), 'Unique sample expected')

self.assertEqual(ss.aggregate().record.sample.shape, (1,2), 'Unique sample expected')

Suggested change

self.assertEqual(ss.record.sample.shape, (1,2), 'Unique sample expected')

covered by other test cases

tests/test_mock_sampler.py

kevinchern

@randomir could you take another look? This should be the draft_final_final; @jackraymond and I approve.

randomir

kwargs overwrite order should be fixed, but generally the code looks good (minus a few nits below).

P.S. since this PR organically extends the idea of exact solver fallback, that's fine, but the switching logic is becoming to look ugly and inelegant, so I think we should generalize it (in a follow-up PR; I'll make an issue).

dwave/system/testing.py

tests/test_mock_sampler.py

Co-authored-by: Radomir Stevanovic <[email protected]>

Change plain text to Sphinx style for documentation Co-authored-by: Radomir Stevanovic <[email protected]>

Co-authored-by: Radomir Stevanovic <[email protected]>

randomir · 2024-10-15T17:47:01Z

I'm happy to merge this; would you like to take a final look, @arcondello?

arcondello

Seems reasonable to me

* WIP - added the option to switch sampler and relevant testing codes * removed the flux biases code * changed the initialization of mocking sampler from init to body * simplified substitude_kwargs * Fix num_qubits bug; raise mocked_parameters and dimod_sampler to class variable status * Add missing property. Make substitute_kwargs a class variable. * Minor corrections * Revert change to support virtual graph composite * num_qubits for pegasus fixed * moved class attributes to instance attributes * updated tests for mock dwave sampler * Revert "updated tests for mock dwave sampler" This reverts commit 89ade5f. * created a local dictionary substitue_kwargs to ensure each instance of mock sampler has its own copies * updated gitignore * Update BQM definition to simplify variable weights Co-authored-by: Jack Raymond <[email protected]> * Replace single-read sample with num_reads=2 in MockDWaveSampler test Co-authored-by: Jack Raymond <[email protected]> * Update test to validate second sample state in MockDWaveSampler Co-authored-by: Jack Raymond <[email protected]> * Remove redundant energy check in MockDWaveSampler test Co-authored-by: Jack Raymond <[email protected]> * Removed redundant comments * Polished formatting and removed changes in .gitignore * Renamed CustomSampler to ConstantSampler in test cases * Updated documentation of MockDWaveSampler * Bugfix: substitute sampler not working as expected * Removed files * Changed shortcircuit to None identity check * Modified None identity check slightly * Add ss.info.update * Added documentation for new parameters * Move comments to documentation * Update exact_solver_cutoff along with its documentation * Correct errors related to misnaming subtitute_* as mock_* * Delete duplicate file accidentally pushed * Add note on return of ascent sampler * Update dwave/system/testing.py Co-authored-by: Radomir Stevanovic <[email protected]> * Update dwave/system/testing.py Change plain text to Sphinx style for documentation Co-authored-by: Radomir Stevanovic <[email protected]> * Update dwave/system/testing.py Co-authored-by: Radomir Stevanovic <[email protected]> * Fixed indentation * Updated comment * Added subtests in TestMockSampler * Modified comments * Minor adjustment for assert statement * Added test for kwargs overwrite in TestMockDWaveSampler --------- Co-authored-by: Andy Zhang <[email protected]> Co-authored-by: Jack Raymond <[email protected]> Co-authored-by: Kevin Chern <[email protected]> Co-authored-by: Radomir Stevanovic <[email protected]>

Andy Zhang and others added 14 commits September 18, 2024 16:10

WIP - added the option to switch sampler and relevant testing codes

7493ffa

removed the flux biases code

b038055

changed the initialization of mocking sampler from init to body

890c0cd

simplified substitude_kwargs

ec11dd1

Fix num_qubits bug; raise mocked_parameters and dimod_sampler to clas…

cba4278

…s variable status

Add missing property. Make substitute_kwargs a class variable.

3ff3b2f

Minor corrections

be73d79

Revert change to support virtual graph composite

1a7d1a8

num_qubits for pegasus fixed

4a22d49

moved class attributes to instance attributes

95907a8

updated tests for mock dwave sampler

89ade5f

Revert "updated tests for mock dwave sampler"

024ea98

This reverts commit 89ade5f.

created a local dictionary substitue_kwargs to ensure each instance o…

b773d54

…f mock sampler has its own copies

merge changes from bugfix

e0b2c7f

updated gitignore

d7e6da0

arcondello requested review from jackraymond, arcondello and kevinchern September 19, 2024 23:56

kevinchern reviewed Sep 20, 2024

View reviewed changes

randomir reviewed Sep 20, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

jackraymond reviewed Sep 20, 2024

View reviewed changes

dwave/system/testing.py Show resolved Hide resolved

jackraymond reviewed Sep 20, 2024

View reviewed changes

jackraymond suggested changes Sep 20, 2024

View reviewed changes

AndyZzzZzzZzz and others added 4 commits September 20, 2024 13:07

Update BQM definition to simplify variable weights

1fbf954

Co-authored-by: Jack Raymond <[email protected]>

Replace single-read sample with num_reads=2 in MockDWaveSampler test

3dd1ce4

Co-authored-by: Jack Raymond <[email protected]>

Update test to validate second sample state in MockDWaveSampler

6e4e5cb

Co-authored-by: Jack Raymond <[email protected]>

Remove redundant energy check in MockDWaveSampler test

466ef49

Co-authored-by: Jack Raymond <[email protected]>

Andy Zhang added 5 commits September 25, 2024 11:05

Changed shortcircuit to None identity check

300fb99

Modified None identity check slightly

0dac9f3

Add ss.info.update

b0765b0

Added documentation for new parameters

b04b494

Move comments to documentation

ae705b2

jackraymond reviewed Sep 27, 2024

View reviewed changes

Andy Zhang and others added 3 commits September 27, 2024 15:44

Update exact_solver_cutoff along with its documentation

0389b60

Correct errors related to misnaming subtitute_* as mock_*

0bacfe1

Delete duplicate file accidentally pushed

74997f9

jackraymond approved these changes Oct 7, 2024

View reviewed changes

kevinchern reviewed Oct 7, 2024

View reviewed changes

Add note on return of ascent sampler

6f3ecd9

kevinchern approved these changes Oct 7, 2024

View reviewed changes

randomir reviewed Oct 9, 2024

View reviewed changes

AndyZzzZzzZzz and others added 9 commits October 10, 2024 14:46

Update dwave/system/testing.py

5600592

Co-authored-by: Radomir Stevanovic <[email protected]>

Update dwave/system/testing.py

894fcd9

Change plain text to Sphinx style for documentation Co-authored-by: Radomir Stevanovic <[email protected]>

Update dwave/system/testing.py

c46359b

Co-authored-by: Radomir Stevanovic <[email protected]>

Fixed indentation

de76c22

Updated comment

b40d778

Added subtests in TestMockSampler

2fce4d4

Modified comments

b3b728c

Minor adjustment for assert statement

8eaf232

Added test for kwargs overwrite in TestMockDWaveSampler

a064358

arcondello approved these changes Oct 15, 2024

View reviewed changes

randomir merged commit 17b142a into dwavesystems:master Oct 15, 2024
21 checks passed

JoelPasvolsky mentioned this pull request Nov 15, 2024

Remove repeated property and update upper limit #545

Merged

randomir mentioned this pull request Dec 2, 2024

Release 1.28.0 #548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature expansion and bug-fix #537

Feature expansion and bug-fix #537

AndyZzzZzzZzz commented Sep 19, 2024 •

edited

Loading

arcondello commented Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

kevinchern commented Sep 19, 2024

kevinchern left a comment •

edited

Loading

kevinchern Sep 20, 2024

jackraymond Sep 20, 2024

kevinchern Sep 20, 2024

jackraymond Sep 20, 2024

kevinchern Sep 20, 2024

kevinchern Sep 20, 2024

jackraymond Sep 20, 2024

jackraymond left a comment

jackraymond Sep 20, 2024

jackraymond Sep 20, 2024

jackraymond Sep 27, 2024

jackraymond Sep 27, 2024

jackraymond left a comment

kevinchern left a comment •

edited

Loading

kevinchern Oct 7, 2024 •

edited

Loading

kevinchern left a comment

randomir left a comment

randomir commented Oct 15, 2024

arcondello left a comment

	self.assertEqual(ss.record.sample.shape, (1,2), 'Unique sample expected')
	self.assertEqual(ss.aggregate().record.sample.shape, (1,2), 'Unique sample expected')

Feature expansion and bug-fix #537

Feature expansion and bug-fix #537

Conversation

AndyZzzZzzZzz commented Sep 19, 2024 • edited Loading