Skip to content

Conversation

@jscanvic
Copy link
Collaborator

@jscanvic jscanvic commented May 24, 2025

Fixes #500

From what I witnessed, black does not flag use of deprecated typing.T types, e.g., typing.Dict instead of dict, but ruff does with the rule UP006.

On a tangential note, maybe it'll be worth integrating a stronger linter like ruff into the CI pipeline at some point. We probably don't want to enforce all of the default rules to avoid making contributing overly painful, but for specific things I believe it might be worth having.

Find offending lines

ruff check --target-version "py39" --select "UP006"

(See ruff.txt for the output)

Fix

Apparently, ruff is able to fix the code automatically but it requires using the option --unsafe-fixes so I'm not sure if it can be trusted. It's what I've used for this PR.

ruff check --unsafe-fixes --fix --target-version "py39" --select "UP006"

After doing that, there remained occurrences of the deprecated types 1) in imports, and 2) in docstrings. I got rid of them both manually using this command to find their presence:

git grep -w -e "Dict" -e "List" -e "Tuple"

ruff.txt (download, excerpt below)

deepinv/datasets/cmrxrecon.py:108:19: UP006 Use `tuple` instead of `Tuple` for type annotation
    |
106 |         mask_generator: Optional[BaseMaskGenerator] = None,
107 |         transform: Optional[Callable] = None,
108 |         pad_size: Tuple[int, int] = (512, 256),
    |                   ^^^^^ UP006
109 |         noise_model: NoiseModel = None,
110 |     ):
    |
    = help: Replace with `tuple`

deepinv/datasets/cmrxrecon.py:171:10: UP006 Use `dict` instead of `Dict` for type annotation
    |
169 |     def _retrieve_metadata(
170 |         self, fname: Union[str, Path, os.PathLike]
171 |     ) -> Dict[str, Any]:
    |          ^^^^ UP006
172 |         """Open file and retrieve metadata
    |
    = help: Replace with `dict`

(Truncated)

Found 90 errors.
No fixes available (90 hidden fixes can be enabled with the `--unsafe-fixes` option).

Checks to be done before submitting your PR

  • python3 -m pytest tests/ runs successfully.
  • black . runs successfully.
  • make html runs successfully (in the docs/ directory).
  • Updated docstrings related to the changes (as applicable).
  • Added an entry to the CHANGELOG.rst.

@codecov
Copy link

codecov bot commented May 24, 2025

Codecov Report

Attention: Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.85%. Comparing base (2298c5d) to head (5c45d5c).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
deepinv/sampling/diffusion_sde.py 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #501      +/-   ##
==========================================
- Coverage   79.86%   79.85%   -0.01%     
==========================================
  Files         187      187              
  Lines       16828    16822       -6     
  Branches     2213     2213              
==========================================
- Hits        13440    13434       -6     
  Misses       2661     2661              
  Partials      727      727              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Andrewwango
Copy link
Collaborator

Thanks @jscanvic for the thorough analysis and I will review this over the next couple of weeks.

What do you think about using mypy for static type checking?

@jscanvic
Copy link
Collaborator Author

Thanks @Andrewwango!

I don't know much about mypy but it seems like a neat piece of software. It'd be interesting to experiment with it and see how we could use it for the library!

@Andrewwango Andrewwango self-requested a review June 13, 2025 12:16
@Andrewwango Andrewwango self-assigned this Jun 13, 2025
Copy link
Collaborator

@Andrewwango Andrewwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

So does your ruff command work?

ruff check --target-version "py39" --select "UP006"

And the the set of errors it brought up is exactly the set of errors you fix here?

If yes, then let's include it in CI!

I have moved the mypy suggestion to #536.

@jscanvic
Copy link
Collaborator Author

It does most of the work the only thing it does not catch is types in docstrings so I handled them manually. It's added!

@Andrewwango Andrewwango merged commit b0fd882 into deepinv:main Jun 16, 2025
4 checks passed
@jscanvic jscanvic deleted the upgrade_deprecated_types branch June 16, 2025 08:25
@Andrewwango Andrewwango mentioned this pull request Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consistent typing

2 participants