.format() coverted to f-strings in registration & restoration subpackage #5510

Larkinnjm1 · 2021-08-12T16:58:17Z

Description

".format()" call on strings objects modified to f-strings within registration and restoration packages of library to partially address issue #5474.

files modified:
modified: skimage/registration/_masked_phase_cross_correlation.py
modified: skimage/registration/tests/test_masked_phase_cross_correlation.py
modified: skimage/restoration/_denoise.py

Checklist

Docstrings for all functions
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.

".format()" of strings objects modified to f-strings within registration and restoration packages of library per issue scikit-image#5474. Committer: Niall Larkin <nialllarkin@nialls-mbp.home> On branch registration_restoration_fstrings Changes to be committed: modified: skimage/registration/_masked_phase_cross_correlation.py modified: skimage/registration/tests/test_masked_phase_cross_correlation.py modified: skimage/restoration/_denoise.py

pep8speaks · 2021-08-12T16:58:20Z

Hello @Larkinnjm1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/restoration/_denoise.py:

Line 193:80: E501 line too long (85 > 79 characters)

Comment last updated at 2021-08-19 15:12:07 UTC

skimage/restoration/_denoise.py

mkcor · 2021-08-14T09:59:13Z

skimage/restoration/_denoise.py

-              "likely to be suboptimal.").format(wavelet.name))
+        warn((f'Wavelet thresholding was designed for use with orthogonal '
+              f'wavelets. For nonorthogonal wavelets such as {wavelet.name}, results are '
+              f'likely to be suboptimal.'))


Isn't there an (unnecessary) extra pair of brackets here?

Thanks for the feedback this is what was in the original code base they held these strings in a tuple so I used the same object as a result. Looking at the warn object from _warnings.py in the _ shared directory there is no impact in using a string directly. However with the fstrings being in a tuple you don't have to include a back slash at the end of each new line of text to avoid getting an IndentationError. Hence putting it in a tuple is potentially less error prone. Is this the potential reason why the code was written like this originally does any of the OGs know?

This is actually already just a string and not a tuple, since there is no comma within the parentheses. I don't think there is a good reason to keep it this way and would just remove the extra parentheses.

Cool thanks for the input @grlee77 and @mkcor I will amend this item then.

mkcor · 2021-08-14T09:59:33Z

skimage/restoration/_denoise.py

-        warn(("Thresholding method {} selected.  The user-specified threshold "
-              "will be ignored.").format(method))
+        warn((f'Thresholding method {method} selected.  The user-specified threshold '
+              f'will be ignored.'))


same comment as above

Per the same response above to the braketing (tuple) object issue.

skimage/registration/tests/test_masked_phase_cross_correlation.py

skimage/registration/_masked_phase_cross_correlation.py

1. Reformatting of strings in the following functions following pull request review from mkcor: - def cross_correlate_masked - def denoise_bilateral 2. Reinstatement of function "def test_deprecated_masked_register_translation" reformatted in error. Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

…nto registration_restoration_fstrings

…m/Larkinnjm1/scikit-image into registration_restoration_fstrings Merged with remote changes made per d9bc06b

On branch registration_restoration_fstrings Changes to be committed: modified: skimage/restoration/_denoise.py _denoise.py updated as a result of review performed by PEP8speaks, mkcor and grlee77 to pull request scikit-image#5510 modifications made: - reduction of line lenght per PEP8 review E501 errors - removal of brackets from f-strings.

hmaarrfk

For concatenating strings, you only need an f on the portions that have fields to format.

hmaarrfk · 2021-08-19T12:05:34Z

Sorry for then noise. grlee already asked for a specific style. Lets follow it. This looks good to me.

grlee77

See suggestions on test case removal. This was the cause of the current CI failure.

The removal was done in a recently merged PR

skimage/registration/tests/test_masked_phase_cross_correlation.py

Updates made to 'test_masked_phase_cross_correlation.py' due to deprecated test method "test_deprecated_masked_register_translation" & "skimge.feature.masked_register_translation" included in error noted during PR review grlee77 Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

grlee77 · 2021-08-19T16:44:25Z

Sorry for then noise. grlee already asked for a specific style. Lets follow it. This looks good to me.

No worries, thanks for reviewing! For reference, we discussed this style choice previously during the SciPy sprint: #5478 (review)

grlee77 · 2021-08-19T16:45:30Z

Thanks @Larkinnjm1, just waiting for CI to finish before merging

grlee77 · 2021-08-19T17:49:37Z

The one CI failure that was observed is due to an unrelated PR that was merged and is resolved by #5522

mkcor reviewed Aug 14, 2021

View reviewed changes

This was referenced Aug 16, 2021

2021's calendar of community management #5169

Closed

.format() in (legacy) code could be converted to f-strings. #5474

Closed

grlee77 added the 🔧 type: Maintenance Refactoring and maintenance of internals label Aug 17, 2021

Larkinnjm1 and others added 4 commits August 18, 2021 15:55

Merge branch 'main' of https://github.com/scikit-image/scikit-image i…

3be43ad

…nto registration_restoration_fstrings

Merge branch 'registration_restoration_fstrings' of https://github.co…

73ad656

…m/Larkinnjm1/scikit-image into registration_restoration_fstrings Merged with remote changes made per d9bc06b

grlee77 approved these changes Aug 19, 2021

View reviewed changes

hmaarrfk reviewed Aug 19, 2021

View reviewed changes

hmaarrfk approved these changes Aug 19, 2021

View reviewed changes

grlee77 requested changes Aug 19, 2021

View reviewed changes

skimage/registration/tests/test_masked_phase_cross_correlation.py Outdated Show resolved Hide resolved

skimage/registration/tests/test_masked_phase_cross_correlation.py Outdated Show resolved Hide resolved

grlee77 approved these changes Aug 19, 2021

View reviewed changes

grlee77 merged commit 118d813 into scikit-image:main Aug 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.format() coverted to f-strings in registration & restoration subpackage #5510

.format() coverted to f-strings in registration & restoration subpackage #5510

Larkinnjm1 commented Aug 12, 2021

pep8speaks commented Aug 12, 2021 •

edited

mkcor Aug 14, 2021

Larkinnjm1 Aug 18, 2021

grlee77 Aug 18, 2021 •

edited

Larkinnjm1 Aug 18, 2021

mkcor Aug 14, 2021

Larkinnjm1 Aug 18, 2021

hmaarrfk left a comment

hmaarrfk commented Aug 19, 2021

grlee77 left a comment

grlee77 commented Aug 19, 2021

grlee77 commented Aug 19, 2021

grlee77 commented Aug 19, 2021

.format() coverted to f-strings in registration & restoration subpackage #5510

.format() coverted to f-strings in registration & restoration subpackage #5510

Conversation

Larkinnjm1 commented Aug 12, 2021

Description

Checklist

For reviewers

pep8speaks commented Aug 12, 2021 • edited

Comment last updated at 2021-08-19 15:12:07 UTC

mkcor Aug 14, 2021

Choose a reason for hiding this comment

Larkinnjm1 Aug 18, 2021

Choose a reason for hiding this comment

grlee77 Aug 18, 2021 • edited

Choose a reason for hiding this comment

Larkinnjm1 Aug 18, 2021

Choose a reason for hiding this comment

mkcor Aug 14, 2021

Choose a reason for hiding this comment

Larkinnjm1 Aug 18, 2021

Choose a reason for hiding this comment

hmaarrfk left a comment

Choose a reason for hiding this comment

hmaarrfk commented Aug 19, 2021

grlee77 left a comment

Choose a reason for hiding this comment

grlee77 commented Aug 19, 2021

grlee77 commented Aug 19, 2021

grlee77 commented Aug 19, 2021

pep8speaks commented Aug 12, 2021 •

edited

grlee77 Aug 18, 2021 •

edited