Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Aug 27, 2024

Conversation

LinasKo
Copy link
Contributor

@LinasKo LinasKo commented Aug 14, 2024

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.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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

  • Docs updated? What were the changes:

* Previously sv.Color.RED would be seen as having type 'classproperty'
* Setting the type info during construvtion passes it to the return type
* Previously unhandled case where the class was missing is now explicitly checked for
* Broke down the inheritance from 'property', as it was marking the result as 'classproperty' instead of e.g. Color, but only until type was provided (but without mypy confusion).
* No longer means 'has default value'. Removed where it meant that.
* `Optional[datatype]` is now used instead of `datatype, optional`
* Fixed a handful of incorrect type annotations
* Because we're generating docs from type annotations of function signatures, I can't find any other way to tell mypy that scene is np.ndarray
@LinasKo
Copy link
Contributor Author

LinasKo commented Aug 14, 2024

Testing checklist:

  • OrientedBoxAnnotator: found regression, fixed it.
  • classproperty: use of Color, ColorPalette
  • Optional in docs
  • got @SkalskiP's opinion on assertions in annotators
  • HeatmapAnnotator
  • Check keypoint docstrings in docs - they should have correct shapes, e.g. (N, M, 2)

@LinasKo LinasKo linked an issue Aug 14, 2024 that may be closed by this pull request
2 tasks
@LinasKo LinasKo changed the title MyPy cleanup + 'optional' docstrings Annotator types and docs: MyPy, consistent 'optional', small tidy-ups Aug 18, 2024
@LinasKo LinasKo marked this pull request as ready for review August 18, 2024 22:47
@LinasKo LinasKo requested review from onuralpszr and SkalskiP August 18, 2024 22:48
@onuralpszr onuralpszr force-pushed the feat/annotators-cleanup branch from 1d42bea to 176fef1 Compare August 19, 2024 23:07
@onuralpszr
Copy link
Collaborator

onuralpszr commented Aug 19, 2024

@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 onuralpszr force-pushed the feat/annotators-cleanup branch 4 times, most recently from 81b4461 to 28e2f1a Compare August 19, 2024 23:55
@onuralpszr
Copy link
Collaborator

onuralpszr commented Aug 19, 2024

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

@LinasKo
Copy link
Contributor Author

LinasKo commented Aug 22, 2024

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 onuralpszr force-pushed the feat/annotators-cleanup branch from 28e2f1a to 8f480ea Compare August 22, 2024 09:14
@onuralpszr
Copy link
Collaborator

onuralpszr commented Aug 22, 2024

@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
Copy link
Contributor Author

LinasKo commented Aug 22, 2024

Yep, happy with that!

supervision/utils/internal.py Show resolved Hide resolved
supervision/annotators/core.py Show resolved Hide resolved
supervision/dataset/core.py Show resolved Hide resolved
@LinasKo
Copy link
Contributor Author

LinasKo commented Aug 26, 2024

@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. #1448 (comment)

@LinasKo LinasKo merged commit 1ae8fc7 into develop Aug 27, 2024
9 checks passed
@onuralpszr onuralpszr deleted the feat/annotators-cleanup branch September 23, 2024 15:09
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.

Invalid validations on KeyPoints class?
3 participants