STYLE: Fix codespell issues #3218

jhlegarreta · 2024-05-12T14:58:40Z

STYLE: Fix typo in view parameter sagital
STYLE: Deprecate l parameter name in favor of step in PIESNO

pep8speaks · 2024-05-12T14:58:46Z

Hello @jhlegarreta, Thank you for updating !

In the file dipy/denoise/tests/test_noise_estimate.py:

Line 77:81: E501 line too long (82 > 80 characters)

Comment last updated at 2024-05-16 14:09:07 UTC

dipy/viz/streamline.py

skoudoro

Hi @jhlegarreta,

Thank yo for this and see below my comments

dipy/denoise/noise_estimate.py

dipy/viz/streamline.py

dipy/viz/tests/test_streamline.py

skoudoro · 2024-05-14T17:31:32Z

Can you also rebase this PR?

Thank you

jhlegarreta · 2024-05-14T19:10:59Z

Can you also rebase this PR?
Done.

The inline exclude pattern has not made it into a release yet:
https://github.com/codespell-project/codespell?tab=readme-ov-file#inline-ignore

And adding sagital to the excluded word list defeats the purpose, so I prefer to have it failing until a new version of codespell is released, bump the version and add the inline exclude rules then.

skoudoro · 2024-05-15T13:36:24Z

The inline exclude pattern has not made it into a release yet:
https://github.com/codespell-project/codespell?tab=readme-ov-file#inline-ignore

I am lost by this PR and not sure to understand what you are trying to do.

to me, this 2 lines send me opposite message:

jhlegarreta · 2024-05-15T13:55:57Z

I am sorry I have lost some lines in previous commits, you will want to check the last push force:
050567a

The codespell inline command would allow to tell to skip the sagital words that we still need to keep for backwards compatibility reasons. That is codespell still complains bout 3 lines containing sagital.

Tests are passing now: failing tests are unrelated.

codecov · 2024-05-15T14:15:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.75%. Comparing base (f741b88) to head (7b16a78).
Report is 5 commits behind head on master.

❗ Current head 7b16a78 differs from pull request most recent head eb861de. Consider uploading reports for the commit eb861de to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3218   +/-   ##
=======================================
  Coverage   83.75%   83.75%           
=======================================
  Files         153      153           
  Lines       21343    21348    +5     
  Branches     3445     3446    +1     
=======================================
+ Hits        17876    17881    +5     
  Misses       2611     2611           
  Partials      856      856

Files	Coverage Δ
dipy/denoise/noise_estimate.py	`79.56% <100.00%> (+0.68%)`	⬆️
dipy/viz/streamline.py	`71.57% <100.00%> (+1.57%)`	⬆️

... and 3 files with indirect coverage changes

skoudoro

Hi @jhlegarreta,

This is almost ready to go. See below my comments.

Thanks @jhlegarreta!

dipy/denoise/noise_estimate.py

dipy/viz/streamline.py

skoudoro · 2024-05-15T19:52:14Z

I am surprise that # codespell:ignore. does not work because this directive has been implemented in 2022 as you can see here: codespell-project/codespell#2400.
There was some releases after that

skoudoro · 2024-05-15T19:53:56Z

Ah ok, it has been merged only on March 10. Let's request a release

jhlegarreta · 2024-05-15T19:56:50Z

It was only merged in March 2024, and there has not been a release yet. So this is ready to go.

skoudoro · 2024-05-16T02:50:33Z

Hi @jhlegarreta,

ok, this PR is ready to go. However, I am not comfortable to merge with Codespell CI's failing and Code Format CI failing.

All the follow-up PR will then always fail. They are currently green.

Not sure what to do here.

Not sure that #3216 will fix that. For how long those CI's will fails after merging this PR?

Maybe we should wait the codespell release before going further
or we can add sagital to the ignore list and create an issue to remove it as soon as a new release appear in codespell

Please share your thought and let's find a solution.

jhlegarreta · 2024-05-16T13:01:16Z

PR #3216 will not fix this issues. I added sagital to the whitelist; codespell is now passing. @skoudoro please open an issue to make it clear that the word should be removed once a new version of codespell is released.

skoudoro · 2024-05-16T13:53:51Z

Ok, thank you for this, and I will create the issue.

Can you fix code format also: see here: https://github.com/dipy/dipy/actions/runs/9112708491/job/25052537528?pr=3218#step:4:126

Fix typo in view parameter `sagital`. Warn about future removal of support. Fixes: ``` Error: ./dipy/viz/streamline.py:10: sagital ==> sagittal Error: ./dipy/viz/streamline.py:45: sagital ==> sagittal Error: ./dipy/viz/tests/test_streamline.py:34: sagital ==> sagittal ``` raised for example in: https://github.com/dipy/dipy/actions/runs/9051840842/job/24868829333?pr=3216#step:4:17 `codespell` does not allow for now inline ignore rules, so add `sagital` to the whitelist to allow the spell check to pass. The word will be removed as soon as a new `codespell` version gets released. Take advantage of the commit to remove the default value from the docstring and rely on the method signature to remove the maintenance burden.

Deprecate `l` parameter name in favor of `step` in PIESNO: check section 2.6 in original paper by Koay et al., 2009 JMRI for further information about its meaning. Fixes: ``` dipy/denoise/noise_estimate.py:26:33: E741 Ambiguous variable name: `l` dipy/denoise/noise_estimate.py:148:5: E741 Ambiguous variable name: `l` ``` raised for example in: https://github.com/dipy/dipy/actions/runs/9051840840/job/24868829334#step:4:117 Take advantage of the commit to remove the default value from the docstring and rely on the method signature to remove the maintenance burden.

jhlegarreta · 2024-05-16T14:11:12Z

Can you fix code format also: see here: https://github.com/dipy/dipy/actions/runs/9112708491/job/25052537528?pr=3218#step:4:126

My bad, had not seen that. Fixed. This is then ready to go.

jhlegarreta · 2024-05-16T16:19:27Z

Failures are unrelated and have been observed in other PRs:
https://github.com/dipy/dipy/actions/runs/9113819127/job/25059288803?pr=3218#step:11:77 (tokenless limit with GitHub)
https://github.com/dipy/dipy/actions/runs/9113819145/job/25056282197?pr=3218#step:6:115 (hdf5 related)
https://github.com/dipy/dipy/actions/runs/9113819156/job/25056298330?pr=3218 (time limit reached)

skoudoro · 2024-05-16T17:10:00Z

Thank you @jhlegarreta ! merging

jhlegarreta commented May 12, 2024

View reviewed changes

dipy/viz/streamline.py Show resolved Hide resolved

jhlegarreta force-pushed the FixCodespellIssues branch 3 times, most recently from 867a7aa to 2876601 Compare May 12, 2024 15:12

jhlegarreta mentioned this pull request May 12, 2024

STYLE: Format code using ruff #3216

Merged

skoudoro reviewed May 14, 2024

View reviewed changes

skoudoro added 📓type:Documentation 📓 type:Maintenance labels May 14, 2024

jhlegarreta force-pushed the FixCodespellIssues branch 2 times, most recently from 7da7795 to e635078 Compare May 14, 2024 19:10

jhlegarreta force-pushed the FixCodespellIssues branch 3 times, most recently from 59432c4 to 28d749d Compare May 15, 2024 13:14

skoudoro reviewed May 15, 2024

View reviewed changes

dipy/denoise/noise_estimate.py Outdated Show resolved Hide resolved

dipy/viz/streamline.py Outdated Show resolved Hide resolved

dipy/viz/streamline.py Outdated Show resolved Hide resolved

dipy/viz/streamline.py Show resolved Hide resolved

jhlegarreta force-pushed the FixCodespellIssues branch from 28d749d to 7b16a78 Compare May 15, 2024 19:33

jhlegarreta force-pushed the FixCodespellIssues branch from 7b16a78 to 9d2c2b9 Compare May 16, 2024 12:57

jhlegarreta added 2 commits May 16, 2024 10:08

jhlegarreta force-pushed the FixCodespellIssues branch from 9d2c2b9 to eb861de Compare May 16, 2024 14:09

skoudoro merged commit 7402d82 into dipy:master May 16, 2024
29 of 32 checks passed

skoudoro mentioned this pull request May 16, 2024

Removesagital from de codespell ignore list. #3223

Open

jhlegarreta deleted the FixCodespellIssues branch May 16, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STYLE: Fix codespell issues #3218

STYLE: Fix codespell issues #3218

jhlegarreta commented May 12, 2024 •

edited

pep8speaks commented May 12, 2024 •

edited

skoudoro left a comment

skoudoro commented May 14, 2024

jhlegarreta commented May 14, 2024

skoudoro commented May 15, 2024

jhlegarreta commented May 15, 2024

codecov bot commented May 15, 2024 •

edited

skoudoro left a comment

skoudoro commented May 15, 2024

skoudoro commented May 15, 2024

jhlegarreta commented May 15, 2024 •

edited

skoudoro commented May 16, 2024

jhlegarreta commented May 16, 2024

skoudoro commented May 16, 2024

jhlegarreta commented May 16, 2024

jhlegarreta commented May 16, 2024

skoudoro commented May 16, 2024

STYLE: Fix codespell issues #3218

STYLE: Fix codespell issues #3218

Conversation

jhlegarreta commented May 12, 2024 • edited

pep8speaks commented May 12, 2024 • edited

Comment last updated at 2024-05-16 14:09:07 UTC

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro commented May 14, 2024

jhlegarreta commented May 14, 2024

skoudoro commented May 15, 2024

jhlegarreta commented May 15, 2024

codecov bot commented May 15, 2024 • edited

Codecov Report

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro commented May 15, 2024

skoudoro commented May 15, 2024

jhlegarreta commented May 15, 2024 • edited

skoudoro commented May 16, 2024

jhlegarreta commented May 16, 2024

skoudoro commented May 16, 2024

jhlegarreta commented May 16, 2024

jhlegarreta commented May 16, 2024

skoudoro commented May 16, 2024

jhlegarreta commented May 12, 2024 •

edited

pep8speaks commented May 12, 2024 •

edited

codecov bot commented May 15, 2024 •

edited

jhlegarreta commented May 15, 2024 •

edited