search

roboflow / supervision

We write your reusable computer vision tools. 💜
https://supervision.roboflow.com
MIT License
24.21k stars 1.8k forks source link

Annotator types and docs: MyPy, consistent 'optional', small tidy-ups #1448

Closed LinasKo closed 2 months ago

LinasKo commented 3 months ago

Description

MyPy was showing many issues in the keypoints/annotators.py and annotators/core.py, so I cleaned it all up.

Where the issues branched into other files, I likewise mopped it up. Here's a Loom. I strongly suggest reviewing one commit at-a-time.

Lastly, there are a few minuscule miscellaneous fixes.

Fixes:

  1. Changed a few return types from np.array to np.ndarray.
  2. VSCode's mypy would always highlight Color.X and ColorPalette.X in red, as it would see it as having type classproperty. Changed it so it correctly sees Color and ColorPalette.
  3. We have many variables marked optional. Now, in annotators, they're all the same - marked Optional[dtype] if and only if they can be None.
  4. ImageType wouldn't be seen as np.ndarray, as we type-annotate it as ImageType. I added assertions, which are the easiest way to tell mypy which one it is.
    • There was also one case of missing decorator
  5. Commit "Add error handling, clean up mypy confusion" addresses multiple issues that were revealed after superficial MyPy issues were cleaned up.
  6. Some examples were missing sv..
  7. Keypoint docs previously said that sv.KeyPoints holds things of shape (N, 2), where it's actually (N, M, 2).

Type of change

Please delete options that are not relevant.

How has this change been tested, please provide a testcase or example of how you tested the change?

Tested locally. Colab: https://colab.research.google.com/drive/1UjRYth4NUfAsvQiz5-CfyRJ9zbXxj2i-?usp=sharing

Any specific deployment considerations

Many docstrings affected in a small way - have a quick look at type signatures, etc.

Docs

LinasKo commented 3 months ago

Testing checklist:

onuralpszr commented 3 months ago

@LinasKo I added my first initial very small change into toml file about reduce mypy error and make sure it does proper mypy check I got them from by using mypy --install-types and added into toml as "optional" so it won't install unless type poetry install --with typechcek so I didn't want to burden anyone to install type check libraries in development.

onuralpszr commented 3 months ago

I removed our "isort" original and params of isort and doing all via ruff and remove extra isort from pre-commit too.

LinasKo commented 2 months ago

Hi @onuralpszr ,

I've had a look at this, and I'd like you to remove your commits for now.

The PR is mostly focused on annotators typing, and is already past the limits of how much should be contained in a single PR.

I see the isort is immediately active, yet if some of the changes need a separate install, it'd likely sit idle as dead code until an indeterminate time in the future, which I don't want.

onuralpszr commented 2 months ago

@LinasKo I backup my changes and I will move to another PR so we can merge mypy/isort formatting changes. Is that for work for you ?

LinasKo commented 2 months ago

Yep, happy with that!

LinasKo commented 2 months ago

@SkalskiP, I've addressed the requests, except for classproperty error message. I think we should keep it as it matches the error raised by property. https://github.com/roboflow/supervision/pull/1448#discussion_r1731121297