Remove consider-using-f-string lint rule and updates #12423

joesho112358 · 2024-05-17T01:36:30Z

Summary

#9614 removing consider-using-f-string

Details and comments

This is a biggie. can break it down into smaller ones if desired... somehow :)

qiskit-bot · 2024-05-17T01:36:37Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

@enavarro51
@Cryoris
@Qiskit/terra-core
@ajavadia
@levbishop
@mtreinish
@nkanazawa1989

qiskit-bot · 2024-05-17T02:35:18Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

@enavarro51
@Cryoris
@Qiskit/terra-core
@ajavadia
@levbishop
@mtreinish
@nkanazawa1989

…into remove-consider-using-f-string

coveralls · 2024-05-17T03:20:30Z

Pull Request Test Coverage Report for Build 9575098874

Details

76 of 131 (58.02%) changed or added relevant lines in 55 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 89.773%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/classicalfunction/boolean_expression.py	0	1	0.0%
qiskit/circuit/equivalence.py	0	1	0.0%
qiskit/circuit/gate.py	1	2	50.0%
qiskit/circuit/tools/pi_check.py	3	4	75.0%
qiskit/dagcircuit/dagdependency.py	3	4	75.0%
qiskit/dagcircuit/dagdependency_v2.py	3	4	75.0%
qiskit/dagcircuit/dagdepnode.py	0	1	0.0%
qiskit/providers/basic_provider/basic_provider_tools.py	0	1	0.0%
qiskit/providers/options.py	0	1	0.0%
qiskit/pulse/transforms/alignments.py	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.11%

Totals
Change from base Build 9571576575:	0.02%
Covered Lines:	63579
Relevant Lines:	70822

💛 - Coveralls

mtreinish

I only got through qiskit/converters so far, but I need to context switch so I decided to leave a comment on the small amount of progress I've made so far. Overall this looks good thanks a ton for doing this, it's often a thankless job to do this kind of modernization.

I'm just curious did you do this all by hand or did you run something like pyupgrade to automate it?

qiskit/circuit/duration.py

mtreinish · 2024-05-29T17:49:50Z

qiskit/circuit/library/graph_state.py

@@ -74,7 +74,7 @@ def __init__(self, adjacency_matrix: list | np.ndarray) -> None:
            raise CircuitError("The adjacency matrix must be symmetric.")

        num_qubits = len(adjacency_matrix)
-        circuit = QuantumCircuit(num_qubits, name="graph: %s" % (adjacency_matrix))
+        circuit = QuantumCircuit(num_qubits, name=f"graph: {adjacency_matrix}")


I did not realize the circuit name for GraphState was embedding a string representation of the adjacency matrix. Lol, that could be quite the string.

qiskit/circuit/library/hamiltonian_gate.py

qiskit/circuit/tools/pi_check.py

mtreinish · 2024-05-29T18:03:19Z

qiskit/circuit/tools/pi_check.py

                    else:
-                        str_out = "{:.{}g}".format(single_inpt, ndigits)
+                        str_out = f"{single_inpt:.{ndigits}g}"


I didn't realize you could nest variables in an fstring like this for a formatter. That's a neat trick.

yeah, not my trick, but i thought it was cool, too! :)

qiskit/circuit/tools/pi_check.py

Co-authored-by: Matthew Treinish <[email protected]>

joesho112358 · 2024-05-29T22:48:45Z

I'm just curious did you do this all by hand or did you run something like pyupgrade to automate it?

by hand

# Conflicts: # pyproject.toml

Cryoris

That certainly was a lot of effort, thanks for tackling this! Just some questions otherwise LGTM.

qiskit/providers/models/backendproperties.py

qiskit/pulse/configuration.py

qiskit/qasm2/export.py

qiskit/synthesis/linear/cnot_synth.py

Cryoris · 2024-06-17T09:40:04Z

qiskit/visualization/circuit/latex.py

In this file there are also some f-string disables, is this due to the mix with Latex? In that case couldn't it be resolved with additional braces, like you already did in some cases?

i think some of these changes initially lead to unit test failures. i will at least narrow it down to make sure everything that can be updated is

nevermind! looks like i may have missed the ide adding/not adding brackets. updates inbound

nevermind again! didn't realize those unit tests were skipped when i ran locally. i'll have to look into it later

Co-authored-by: Julien Gacon <[email protected]>

Cryoris

LGTM thanks for all the effort!

* remove consider-using-f-string lint rule and updates * reverting a latex update * f-string update based on review * Update qiskit/circuit/library/hamiltonian_gate.py Co-authored-by: Matthew Treinish <[email protected]> * Update qiskit/circuit/tools/pi_check.py Co-authored-by: Matthew Treinish <[email protected]> * Update qiskit/circuit/tools/pi_check.py Co-authored-by: Matthew Treinish <[email protected]> * updates after merge * Update qiskit/providers/models/backendproperties.py Co-authored-by: Julien Gacon <[email protected]> * Update qiskit/synthesis/linear/cnot_synth.py Co-authored-by: Julien Gacon <[email protected]> * updates from PR * latex fixes --------- Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Julien Gacon <[email protected]>

remove consider-using-f-string lint rule and updates

0cffac2

joesho112358 requested review from nonhermitian, a team, eggerdj, wshanks, alexanderivrii, ShellyGarion and jyu00 as code owners May 17, 2024 01:36

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label May 17, 2024

Merge branch 'main' into remove-consider-using-f-string

bcabddc

joesho112358 marked this pull request as draft May 17, 2024 02:03

joesho112358 marked this pull request as ready for review May 17, 2024 02:35

joesho112358 added 2 commits May 16, 2024 23:00

reverting a latex update

0c6a653

Merge remote-tracking branch 'origin/remove-consider-using-f-string' …

3755703

…into remove-consider-using-f-string

mtreinish reviewed May 29, 2024

View reviewed changes

joesho112358 and others added 5 commits May 29, 2024 18:36

f-string update based on review

1170e6c

Update qiskit/circuit/library/hamiltonian_gate.py

92e95cb

Co-authored-by: Matthew Treinish <[email protected]>

Update qiskit/circuit/tools/pi_check.py

bfe70b8

Co-authored-by: Matthew Treinish <[email protected]>

Update qiskit/circuit/tools/pi_check.py

c4d7169

Co-authored-by: Matthew Treinish <[email protected]>

Merge branch 'main' into remove-consider-using-f-string

6fafa46

joesho112358 added 2 commits June 16, 2024 22:00

Merge branch 'main' into remove-consider-using-f-string

a8559ab

# Conflicts: # pyproject.toml

updates after merge

fb30dc9

Cryoris reviewed Jun 17, 2024

View reviewed changes

joesho112358 and others added 3 commits June 17, 2024 22:05

Update qiskit/providers/models/backendproperties.py

8e09ad4

Co-authored-by: Julien Gacon <[email protected]>

Update qiskit/synthesis/linear/cnot_synth.py

a1d088b

Co-authored-by: Julien Gacon <[email protected]>

updates from PR

1c13ea4

joesho112358 added 3 commits June 17, 2024 22:51

Merge branch 'main' into remove-consider-using-f-string

adb8d08

Merge branch 'main' into remove-consider-using-f-string

ebec03b

latex fixes

397d6e7

Cryoris added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels Jun 19, 2024

Cryoris approved these changes Jun 19, 2024

View reviewed changes

Cryoris added this pull request to the merge queue Jun 19, 2024

Merged via the queue into Qiskit:main with commit 53667d1 Jun 19, 2024
15 checks passed

joesho112358 deleted the remove-consider-using-f-string branch June 19, 2024 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove consider-using-f-string lint rule and updates #12423

Remove consider-using-f-string lint rule and updates #12423

joesho112358 commented May 17, 2024

qiskit-bot commented May 17, 2024

qiskit-bot commented May 17, 2024

coveralls commented May 17, 2024 •

edited

Loading

mtreinish left a comment

mtreinish May 29, 2024

mtreinish May 29, 2024

joesho112358 May 29, 2024

joesho112358 commented May 29, 2024

Cryoris left a comment

Cryoris Jun 17, 2024

joesho112358 Jun 18, 2024

joesho112358 Jun 18, 2024

joesho112358 Jun 18, 2024

Cryoris left a comment

Remove consider-using-f-string lint rule and updates #12423

Remove consider-using-f-string lint rule and updates #12423

Conversation

joesho112358 commented May 17, 2024

Summary

Details and comments

qiskit-bot commented May 17, 2024

qiskit-bot commented May 17, 2024

coveralls commented May 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9575098874

Details

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish May 29, 2024

Choose a reason for hiding this comment

mtreinish May 29, 2024

Choose a reason for hiding this comment

joesho112358 May 29, 2024

Choose a reason for hiding this comment

joesho112358 commented May 29, 2024

Cryoris left a comment

Choose a reason for hiding this comment

Cryoris Jun 17, 2024

Choose a reason for hiding this comment

joesho112358 Jun 18, 2024

Choose a reason for hiding this comment

joesho112358 Jun 18, 2024

Choose a reason for hiding this comment

joesho112358 Jun 18, 2024

Choose a reason for hiding this comment

Cryoris left a comment

Choose a reason for hiding this comment

coveralls commented May 17, 2024 •

edited

Loading