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

Feature/belated pep8 compliance #56

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jackraymond
Copy link
Contributor

Pull requests 51, 45 (to predecessor dwave-neal), and 25 introduced a large number of PEP8 violations, this pull request corrects those errors (with some small amount of compliance collateral not related to the pull requests).
In this pull request I made several files code complaint with pep8 and pyflakes3.

I plead guilty to charges E501, E265, E231, E261, E262, E251, E225, E112, W293 and others. Now that I program primarily in python I see the ugliness.

I standardized sa/sampler.py and tabu/sampler.py at width <=79 wrap around (with a few very difficult to compress exceptions at <99). The code was close to <=79 convention in most places, so this seemed sensible.
For test_simulated_annealing_sampler() and test_tabu_sampler() I allowed many lines <=99 to remain as written, except where local style consistency dictated their shortening.

The changes for the most part errors I introduced in pull requests, in a handful of cases they potentially override stylistic choices of other contributers. Apologies for this! Also apologies to my main reviewers the last 2 years.

@arcondello arcondello self-requested a review June 30, 2023 17:52
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #56 (51bc6ae) into main (f412d64) will increase coverage by 1.13%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   93.02%   94.16%   +1.13%     
==========================================
  Files          17       17              
  Lines         545      531      -14     
==========================================
- Hits          507      500       -7     
+ Misses         38       31       -7     
Impacted Files Coverage Δ
dwave/samplers/sa/sampler.py 88.88% <73.33%> (+3.92%) ⬆️
dwave/samplers/tabu/sampler.py 98.07% <100.00%> (+0.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

For test_simulated_annealing_sampler() and test_tabu_sampler() I allowed many lines <=99 to remain as written, except where local style consistency dictated their shortening.

It looks like a lot of lines with <100 characters got changed to <80. IMO it would be better to stick to <100 and minimize the changes. Also, 100 is (IMO) much more readable on modern systems than 80.

dwave/samplers/sa/sampler.py Outdated Show resolved Hide resolved
dwave/samplers/sa/sampler.py Outdated Show resolved Hide resolved
dwave/samplers/sa/sampler.py Outdated Show resolved Hide resolved
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.

2 participants