Replace format string to f-strings in python file#990
Replace format string to f-strings in python file#990kenya-sk wants to merge 14 commits intotensorflow:masterfrom
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
… into replace-to-f-strings
|
@kenya-sk Thank you for doing this. This PR seems to mix (a) converting format strings to be f-strings and (b) other format changes from Yapf. This PR's title and description only reflect the changes in (a). Per discussion on issue #983, it would be better to split up the two kinds of changes. So, could you pull out the non-f-string changes to another PR? |
|
@mhucka If it would be better to keep this PRs separated even before merging, I'm happy to adjust accordingly. |
|
Ah, I see. In that case, I'll work with you on getting #991 submitted. I have a question on that one (asked in the comments on that PR). |
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces .format() string formatting with f-strings across multiple Python files, which is a good modernization effort. The changes are mostly correct and improve readability. I've made a few suggestions to further improve the readability of some of the new f-strings, particularly where they are split across multiple lines or constructed from multiple concatenated f-strings.
| SRC, "reports/", f"CliffordBenchmarks.benchmark_clifford_circuit_" | ||
| f"{params.n_qubits}_{params.n_moments}_{params.batch_size}") |
There was a problem hiding this comment.
For readability, you can combine these two concatenated f-strings into a single one. This makes the code cleaner and easier to understand.
| SRC, "reports/", f"CliffordBenchmarks.benchmark_clifford_circuit_" | |
| f"{params.n_qubits}_{params.n_moments}_{params.batch_size}") | |
| SRC, "reports/", | |
| f"CliffordBenchmarks.benchmark_clifford_circuit_{params.n_qubits}_{params.n_moments}_{params.batch_size}") |
| res.name, f"CliffordBenchmarks.benchmark_clifford_circuit_" | ||
| f"{params.n_qubits}_{params.n_moments}_{params.batch_size}") |
There was a problem hiding this comment.
Similar to the previous comment, combining these concatenated f-strings into a single f-string will improve code clarity.
| res.name, f"CliffordBenchmarks.benchmark_clifford_circuit_" | |
| f"{params.n_qubits}_{params.n_moments}_{params.batch_size}") | |
| res.name, | |
| f"CliffordBenchmarks.benchmark_clifford_circuit_{params.n_qubits}_{params.n_moments}_{params.batch_size}") |
| f"RandomCircuitBenchmarks.benchmark_random_circuit_" | ||
| f"{params.n_rows}_{params.n_cols}_{params.n_moments}") |
There was a problem hiding this comment.
To improve readability, it's better to use a single f-string here instead of concatenating two.
f"RandomCircuitBenchmarks.benchmark_random_circuit_{params.n_rows}_{params.n_cols}_{params.n_moments}")| res.name, f"RandomCircuitBenchmarks.benchmark_random_circuit_" | ||
| f"{params.n_rows}_{params.n_cols}_{params.n_moments}") |
There was a problem hiding this comment.
For better code clarity, you can combine these two f-strings into one.
res.name,
f"RandomCircuitBenchmarks.benchmark_random_circuit_{params.n_rows}_{params.n_cols}_{params.n_moments}")| raise TypeError('num_samples tensor must have the same shape ' | ||
| 'as pauli_sums tensor. got: {} expected: {}'.format( | ||
| num_samples.shape, pauli_sums.shape)) | ||
| f'as pauli_sums tensor. got: {num_samples.shape}' | ||
| f' expected: {pauli_sums.shape}') |
There was a problem hiding this comment.
This string construction is a mix of a regular string and two f-strings. For better readability and consistency, it would be cleaner to use a single f-string enclosed in parentheses to handle the line break.
| raise TypeError('num_samples tensor must have the same shape ' | |
| 'as pauli_sums tensor. got: {} expected: {}'.format( | |
| num_samples.shape, pauli_sums.shape)) | |
| f'as pauli_sums tensor. got: {num_samples.shape}' | |
| f' expected: {pauli_sums.shape}') | |
| raise TypeError(( | |
| 'num_samples tensor must have the same shape ' | |
| f'as pauli_sums tensor. got: {num_samples.shape} ' | |
| f'expected: {pauli_sums.shape}')) |
| raise ValueError('unexpected signature for analytic_op. ' | ||
| 'Given arg: {}.'.format(str(key)) + '' | ||
| 'The signature should contain: {}.'.format( | ||
| list(expected_signature)) + '' | ||
| ' Given: {}'.format(list(signature)) + '' | ||
| f'Given arg: {key}.' + '' | ||
| 'The signature should contain: ' | ||
| f'{list(expected_signature)}' + '' | ||
| f' Given: {list(signature)}' + '' | ||
| 'Note: noisy ops should use sampled_op') |
There was a problem hiding this comment.
The string concatenation with '' is unnecessary. You can combine these into a single f-string for better readability.
| raise ValueError('unexpected signature for analytic_op. ' | |
| 'Given arg: {}.'.format(str(key)) + '' | |
| 'The signature should contain: {}.'.format( | |
| list(expected_signature)) + '' | |
| ' Given: {}'.format(list(signature)) + '' | |
| f'Given arg: {key}.' + '' | |
| 'The signature should contain: ' | |
| f'{list(expected_signature)}' + '' | |
| f' Given: {list(signature)}' + '' | |
| 'Note: noisy ops should use sampled_op') | |
| raise ValueError( | |
| f'unexpected signature for analytic_op. ' | |
| f'Given arg: {key}. ' | |
| 'The signature should contain: ' | |
| f'{list(expected_signature)} ' | |
| f' Given: {list(signature)} ' | |
| 'Note: noisy ops should use sampled_op') | |
| raise ValueError('unexpected signature for sampled_op. ' | ||
| 'Given arg: {}.'.format(str(key)) + '' | ||
| 'The signature should contain: {}.'.format( | ||
| list(expected_signature))) | ||
| f'Given arg: {key}.' + '' | ||
| 'The signature should contain: ' | ||
| f'{list(expected_signature)}') |
There was a problem hiding this comment.
The string concatenation with '' can be removed by combining these into a single f-string, which improves readability.
| raise ValueError('unexpected signature for sampled_op. ' | |
| 'Given arg: {}.'.format(str(key)) + '' | |
| 'The signature should contain: {}.'.format( | |
| list(expected_signature))) | |
| f'Given arg: {key}.' + '' | |
| 'The signature should contain: ' | |
| f'{list(expected_signature)}') | |
| raise ValueError( | |
| f'unexpected signature for sampled_op. ' | |
| f'Given arg: {key}. ' | |
| 'The signature should contain: ' | |
| f'{list(expected_signature)}') | |
| raise ValueError( | ||
| "Attempted to scan for symbols in circuit with unsupported" | ||
| " ops inside.", "Expected op found in " | ||
| "tfq.util.get_supported_gates but found: {}.".format(str(op)), | ||
| "Attempted to scan for symbols in circuit with unsupported " | ||
| "ops inside. Expected op found in " | ||
| f"tfq.util.get_supported_gates but found: {op}. " | ||
| "Please make sure circuits contain only ops found in " | ||
| "tfq.util.get_supported_gates().") |
There was a problem hiding this comment.
This ValueError message is constructed from multiple string literals. It can be simplified into a single f-string for better readability.
| raise ValueError( | |
| "Attempted to scan for symbols in circuit with unsupported" | |
| " ops inside.", "Expected op found in " | |
| "tfq.util.get_supported_gates but found: {}.".format(str(op)), | |
| "Attempted to scan for symbols in circuit with unsupported " | |
| "ops inside. Expected op found in " | |
| f"tfq.util.get_supported_gates but found: {op}. " | |
| "Please make sure circuits contain only ops found in " | |
| "tfq.util.get_supported_gates().") | |
| raise ValueError( | |
| f"Attempted to scan for symbols in circuit with unsupported " | |
| f"ops inside. Expected op found in " | |
| f"tfq.util.get_supported_gates but found: {op}. " | |
| "Please make sure circuits contain only ops found in " | |
| "tfq.util.get_supported_gates().") |
3 participants
The Python program's strings were written in format strings, so I wrote them using the f-strings recommended in recent Python versions.
No functional changes were made during this rewrite. It was limited to syntactic changes.