-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[WIP] Add ruff configuration to pyproject.toml #9586
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
Changes from 13 commits
2da0e50
535a583
adc0fba
57ef618
462b3ba
812cda8
d577c4c
e652cb3
e9735d3
a6fd291
ae83178
30fed92
a30f15d
7f3c267
4a75693
56c224f
49350eb
a2f12ff
6c47b75
bed8619
0ecbe91
de596c1
785e917
7cadea3
cdc0abd
52af6ee
9e75659
b8861da
93a2bd0
cca56b9
38e405b
90d3111
f9dc386
4dc3362
342ab3d
1aad471
e235863
3d01dec
ff23ac9
1f45ccb
5b3878f
344b4fb
e077b53
a4e20d4
824802f
cdef401
3988d89
ec19280
77c37d0
fa6cc0d
24fe525
26acdc5
ff4c127
1a8fe2f
7417e06
2863fd4
340071b
10db888
79a8577
4a5a077
1feaf3e
d4d029c
9b6fd2c
8097467
934915f
d2d5a12
880faff
08fd9ec
377ae17
492a248
e8db0e6
6ac5db8
e3fd158
8f0c9bf
5a74db6
2b91f8f
90aba9a
0662c05
148f28f
2c0f65d
9afc502
7d1f185
0783aff
de395df
d79662d
3f35fdd
7a2889e
d016769
55f408f
20c854d
ffaf874
d590b72
f02371b
adcd907
97ebcb5
2d659c5
80b078b
03f4b6b
b60fc61
cb725b0
83b8ddf
7b2ca00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,20 +39,20 @@ def bisect_max(f, a, b, steps=50, minwidth=1e-12, retval=False): | |
m = (a + b) / 2 | ||
fm = 0 | ||
while it < steps and b - a > minwidth: | ||
l, r = (a + m) / 2, (m + b) / 2 | ||
fl, fm, fr = f(l), f(m), f(r) | ||
left, r = (a + m) / 2, (m + b) / 2 | ||
fl, fm, fr = f(left), f(m), f(r) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should disable whatever rule forbade this and revert this and all similar changes - it's not needed code churn, and makes this code block harder to read because there's now a split between using Personally I'm not sure I agree with enforcing variable-name conventions by automated checker anyway, because my experience is that there's more exceptions than they're worth, but I don't feel super strongly in general. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I think we've discussed this at least once in the past. For example, it can be much easier to reference an algorithm in a paper if you use the same symbols in the code. And the exceptions add clutter. I also don't feel strongly about whether to use a checker. Having said that, I often find that names are poorly chosen, and can make code difficult to read and search (Could use a better IDE in this case, I suppose) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed (reverted the commits) in #9586 (commits) |
||
|
||
# fl is the maximum | ||
if fl > fm and fl > fr: | ||
b = m | ||
m = l | ||
m = left | ||
# fr is the maximum | ||
elif fr > fm and fr > fl: | ||
a = m | ||
m = r | ||
# fm is the maximum | ||
else: | ||
a = l | ||
a = left | ||
b = r | ||
|
||
it += 1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,7 +115,7 @@ def minimize( | |
var_lb = np.array([-np.inf] * x0.size) | ||
var_ub = np.array([np.inf] * x0.size) | ||
else: | ||
var_lb = np.array([l for (l, _) in bounds]) | ||
var_lb = np.array([lower for (lower, _) in bounds]) | ||
var_ub = np.array([u for (_, u) in bounds]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another case where the rename introduces a split between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, whatever is chosen, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See commits mentioned in previous conversation. |
||
|
||
x, fun, nfev, njev = self.ls_optimize(x0.size, fun, x0, var_lb, var_ub) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,8 +97,8 @@ def minimize( | |
bounds = [(None, None)] * x0.size | ||
|
||
threshold = 3 * np.pi | ||
low = [(l if l is not None else -threshold) for (l, u) in bounds] | ||
high = [(u if u is not None else threshold) for (l, u) in bounds] | ||
low = [(lower if lower is not None else -threshold) for (lower, u) in bounds] | ||
high = [(u if u is not None else threshold) for (lower, u) in bounds] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent naming conventions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted in 0783aff |
||
|
||
name = self._optimizer_names[self.get_nlopt_optimizer()] | ||
opt = nlopt.opt(name, len(low)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,8 +128,8 @@ def minimize( | |
threshold = 2 * np.pi | ||
if bounds is None: | ||
bounds = [(-threshold, threshold)] * x0.size | ||
low = [(l if l is not None else -threshold) for (l, u) in bounds] | ||
high = [(u if u is not None else threshold) for (l, u) in bounds] | ||
low = [(lower if lower is not None else -threshold) for (lower, u) in bounds] | ||
high = [(u if u is not None else threshold) for (lower, u) in bounds] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent naming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted in 0783aff |
||
|
||
def optimize_runner(_queue, _i_pt): # Multi-process sampling | ||
_sol, _opt, _nfev = self._optimize(fun, _i_pt, jac, bounds) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,7 @@ def decomposition_hop(target_coordinate, strengths): | |
specialized_polytope = PolytopeData( | ||
convex_subpolytopes=[ | ||
ConvexPolytopeData( | ||
inequalities=[[v, h, l] for ((h, l), v) in coefficient_dict.items()] | ||
inequalities=[[v, h, ineq2] for ((h, ineq2), v) in coefficient_dict.items()] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a particularly difficult to parse naming change for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been reverted.
|
||
) | ||
] | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,10 +37,10 @@ def count_keys(num_qubits: int) -> List[str]: | |
|
||
def complete_meas_cal( | ||
qubit_list: List[int] = None, | ||
qr: Union[int, List["QuantumRegister"]] = None, | ||
cr: Union[int, List["ClassicalRegister"]] = None, | ||
qr: Union[int, List["QuantumRegister"]] = None, # noqa: F821 | ||
cr: Union[int, List["ClassicalRegister"]] = None, # noqa: F821 | ||
circlabel: str = "", | ||
) -> Tuple[List["QuantumCircuit"], List[str]]: | ||
) -> Tuple[List["QuantumCircuit"], List[str]]: # noqa: F821 | ||
""" | ||
Return a list of measurement calibration circuits for the full | ||
Hilbert space. | ||
|
@@ -114,10 +114,10 @@ def complete_meas_cal( | |
|
||
def tensored_meas_cal( | ||
mit_pattern: List[List[int]] = None, | ||
qr: Union[int, List["QuantumRegister"]] = None, | ||
cr: Union[int, List["ClassicalRegister"]] = None, | ||
qr: Union[int, List["QuantumRegister"]] = None, # noqa: F821 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docs say that F821 is an "undefined name error" -- do you think this is that because the type is in quotes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. That's it. I did not track down the reasoning. Of course, the name is in quotes precisely because the symbol is not defined. Maybe some people don't think this should be done, so there is a rule ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
cr: Union[int, List["ClassicalRegister"]] = None, # noqa: F821 | ||
circlabel: str = "", | ||
) -> Tuple[List["QuantumCircuit"], List[List[int]]]: | ||
) -> Tuple[List["QuantumCircuit"], List[List[int]]]: # noqa: F821 | ||
""" | ||
Return a list of calibration circuits | ||
|
||
|
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.
Per the ruff docs and some of your comments on docstrings in #1179 I'm wondering if we want to add
here too
https://github.com/charliermarsh/ruff#does-ruff-support-numpy--or-google-style-docstrings
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.
That's certainly worth trying.
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 added some doc string checks. But in order to make this more manageable, I plan to instead defer them to a later PR (provided we actually make a switch to ruff) However, several doc string errors/suggestions made by ruff have been merged in recent PRs.