I really like the repo, and I'd suggest to do the following things to it to make it modern, more maintainable, dev friendly and readable:
type the whole thing - right now the repository is half-typed, and some types are wrong (most annoying one is probably NMSConfig which should be floats instead of ints) etc. That way you make it less error-prone, nicer to read and understand, and more developer friendly (especially us that use something like PyCharm).
fix typos of some variables and classes
use argument names as much as possible - for example instead of Conv(in_channels, n_classes, 1) let's make it Conv(in_channels=in_channels, out_channels=out_channels, kernel_size=kernel_size. This makes it longer, but in my opinion way readable, and again less error-prone (the bug @ilijad found and made a recent PR would have never been there is this was implemented)
make test directory not depend on location where you run it from - I made a lazy PR some time ago about it with a bad solution to it. But TLDR: I would just put something like test_directory=Path(__file__).parent in conftest.py and do all path references relative to that, instead of using relative paths from where the tests are run (which makes tests fail if run from any other than test directory.
migrate to Poetry instead of using old pip / requirements.txt - this makes it better for dependency management, freezes dependencies in lock file (so resolution is quicker) but also much more capable of being updates since project that might use your project as a library will use manifest (TOML) file and have a nice range of
use builtin types - this has been a thing in Python since 3.9 - for example instead of importing List, Dict from typing I'd use list[tuple[]] for example.
This are suggestions I would like to apply to your project to make it way more dev friendly and up to some industry standard. I'd be glad to make PRs for any of those you might agree on, but please first let us know in which direction you'd like to go, and a little bit of guarantee if I make a PR that you will indeed review it and give your feedback and potentially merge it.
Please give me some feedback on these suggestions!
The idea looks great! However, I have a few points to address:
Type the whole thing: Thanks for the reminder! I'll be scanning the variable types again to ensure everything is correct.
Fix typos: As mentioned above, I'll try to find and fix any typos, but some might still slip through. I hope others can help catch anything I miss.
Argument checking: I'll also run checks with some linters to ensure everything is consistent.
Using poetry & built-in types: This is a bit tricky. Object detection is a popular application in robotics, and some robots' local environments still require Python 3.8 or have complex requirements. I used built-in types before, but I wanted this repo to be flexible and even support Python 3.6, so I switched back to typing.
Hi @henrytsui000 !
I really like the repo, and I'd suggest to do the following things to it to make it modern, more maintainable, dev friendly and readable:
Conv(in_channels, n_classes, 1)
let's make itConv(in_channels=in_channels, out_channels=out_channels, kernel_size=kernel_size
. This makes it longer, but in my opinion way readable, and again less error-prone (the bug @ilijad found and made a recent PR would have never been there is this was implemented)test_directory=Path(__file__).parent
in conftest.py and do all path references relative to that, instead of using relative paths from where the tests are run (which makes tests fail if run from any other than test directory.list[tuple[]]
for example.This are suggestions I would like to apply to your project to make it way more dev friendly and up to some industry standard. I'd be glad to make PRs for any of those you might agree on, but please first let us know in which direction you'd like to go, and a little bit of guarantee if I make a PR that you will indeed review it and give your feedback and potentially merge it.
Please give me some feedback on these suggestions!