-
Notifications
You must be signed in to change notification settings - Fork 64
Refresh samplers #589
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
Refresh samplers #589
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #589 +/- ##
==========================================
- Coverage 89.38% 86.93% -2.46%
==========================================
Files 22 22
Lines 1997 1997
==========================================
- Hits 1785 1736 -49
- Misses 212 261 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| Note: we need to partially replicate | ||
| :class:`dwave.cloud.Client.get_solver` logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the purpose of this change? Line length was perfectly fine, even shorter then some around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was 91 characters and PEP8 says "it is okay to increase the line length limit up to 99 characters, provided that comments and docstrings are still wrapped at 72 characters"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but our contributing guide says:
- Use a consistent character limit of 100.
| The method blocks for all the currently scheduled work (sampling | ||
| requests) to finish. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, turns out this is not technically true anymore (see dwavesystems/dwave-cloud-client#712), but this PR is maybe not the right place to address that? Or is it?
| # scaling | ||
| inv_scalar = max(min_coupling_range / min_lim, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we ever explicitly said this in our contributing guide, but we generally try to avoid making such whitespace-only changes, as they affect git history for little to no benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are a lot of them, I try to do them all in a single commit. Do we want to just leave them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave them, and "fix" as part of a bigger change of that line or block.
No need to revert this one, though.
Co-authored-by: Radomir Stevanovic <[email protected]>
Refresh of samplers docstrings from a few small time windows over the last weeks. Changes to how the docs are generated/automation (e.g. PR-585 out of scope of this PR.