dwavesystems / dwave-system

An API for easily incorporating the D-Wave system as a sampler, either directly or through Leap's cloud-based hybrid samplers
https://docs.ocean.dwavesys.com/
Apache License 2.0
87 stars 61 forks source link

Another parameter that should be ignored in `MockDWaveSampler` #510

Closed VolodyaCO closed 6 months ago

VolodyaCO commented 6 months ago

Apparently, DWaveSampler accepts x_simple_anneal_time, but MockDWaveSampler does not. This PR just adds this argument to the list of arguments that should be ignored when passed to MockDWaveSampler.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0ae6d26) 90.85% compared to head (079184b) 87.74%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #510 +/- ## ========================================== - Coverage 90.85% 87.74% -3.11% ========================================== Files 24 24 Lines 1673 1673 ========================================== - Hits 1520 1468 -52 - Misses 153 205 +52 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

arcondello commented 6 months ago

x_* parameters are used for one-off projects and experimental features. For that reason, we do not include them in the MockDWaveSampler. You could try setting parameter_warnings=False if you want to test with x_* parameters.

arcondello commented 6 months ago

Ah, nevermind, I see that we raise an error for unknown kwargs.

IMO the better change would be to raise a warning rather than an error in that case.

VolodyaCO commented 6 months ago

@arcondello In that case, do I modify the code to make the change "raising error -> throwing a warning"?

arcondello commented 6 months ago

For now, for your own testing, you can just do

MockDWaveSampler.parameters.update(x_simple_anneal_time=[])

before the main code.

If you're interested in making a deeper fix, I think the backwards compatible thing would be to add a ignore_unknown_parameters=False class-level keyword argument to go with parameter_warnings.

Then https://github.com/dwavesystems/dwave-system/blob/0ae6d260514eaa8f1c967955c6438fbda30c9154/dwave/system/testing.py#L317-L323 would be changed to something like

for kw in kwargs:
    if kw in self.parameters:
        if self.parameter_warnings and kw not in mocked_parameters:
            warnings.warn(f'{kw!r} parameter is valid for DWaveSampler(), '
                                  'but not mocked in MockDWaveSampler().')
    elif not self.ignore_unknown_parameters:
        raise ValueError(f'kwarg {kw!r} invalid for MockDWaveSampler()')

otherwise I can open an issue and we'll get to it sometime relatively soon.

arcondello commented 6 months ago

Or even better would be to add a parameters keyword argument, to go alongside properties that would allow the user to add additional parameters. Basically formalizing

MockDWaveSampler.parameters.update(x_simple_anneal_time=[])

as suggested above.

VolodyaCO commented 6 months ago

I'm popping x_* parameters before passing them to the MockDWaveSampler instead. If x_* are related to experimental features, there's no reason to modify the source code, then. Thanks a lot for the help.