smttsp / temporal_consistency_odt Goto Github PK

Hi, here's some feedback regarding the repo. Maybe it is too strict or nit-picky in some points, but it is what I would notice when you give it to me as showcase before a job interview and probably bring up some of them in the interview to discuss. But don't get me wrong, your project is already in the top 5% tier of projects I see in resumes :-) Also you are probably aware of a lot of these points already and it's a prototype / work-in-progress as I understand. I just looked over it quickly without understanding what really happens in code, so don't expect it to be completely or 100% correct :-)

Project Structure:

1. ✅ The readme is overall great as it captures the idea of the tool very well and motivates the reader to try it out! One thing I noticed though is that the installation instructions were not super-clear to me. I know poetry a little, but I haven't used pyenv or direnv. So I didn't quite understand if I should go pyenv or direnv (or both is necessary).

2. I think it would be awesome if you can provide some minimal example that users can directly execute (like add a video file and instructions how to run your tool on that).

3. ✅ If possible, I'd suggest to remove the setup.py and use the pyproject.toml exclusively. Right now the version in the setup.py is already outdated, and I'm not sure why it is necessary. Maybe then you can also get rid of pytest.ini. I'm a fan of pyproject.toml :-)

4. Suggestion: Expose the main.py as entrypoint (I think in poetry it is [tool.poetry.scripts]) and give it a nice name. This way if users pip-install your package, they can run it conveniently from the shell.

5. ✅ The .python-version file states python 3.11.5, so I understand this is the version you are developing in / for(?) Not many users have this version though. I'd suggest run unit tests for earlier versions as well (e.g. with tox), or explicitly state this version as required in the pyproject.toml

6. ✅ "tests_unit" is a name for a unit test folder that I haven't seen yet. I'd just call it "tests" like most python projects do.

7. ❎ I like the use of black and isort. Note if you use both together, isort recommends using the black profile (profile = "black"). If you want to go one step further, add them as githooks using the pre-commit library https://pre-commit.com/ This is really easy (just add one additional dev-dependency and a simple yaml config file) and it would be a nice addition to the showcase.

-> I couldn't figure this out. had some issues. may try later

Code:

8. This is debatable, but for docstrings I personally find "imperative" text more readable in many cases. E.g. "Assess if the interpreter..." vs "Assesses if the interpreter..." But I like the docstrings as they help understanding and are not just repeating the function name.

9. ✅ I also like the use of typehints. If you use typehints, you might consider using mypy to ensure correctness and consistency. In my experience, while the typehints are initially correct, they tend to not get updated reliably when code is changed :-) Also using mypy helped me reveal bugs in our code that were difficult to cover using unit tests.

10. ❎ I noticed that in the frame_anomaly_detection.py, most (all?) functions take both object_id and track_info as argument. Not sure, but it might make sense to refactor that, e.g. put both in one object if they are used together so frequently.

-> Alternative solutions are not better than this

11. ✅ If you want more feedback, you could run your code through pylint or some other linter. Oh, these can also be added easily as git hooks using pre-commit :-)

It looks like a long list, but it's details, and parts of what I wrote is possibly debatable or plain wrong :-) As I understand you are trying to get a new job now, I'd recommend you to use this project to emphasize your abilities to

do something self-taught and be motivated to stick to it

actively ask for feedback

This is much more relevant than any issues in the code :-)

This repo is not efficient at all and memory usage is too high especially when a video is too long; longer than 1fps.

Save the frames on disk instead of keeping them in memory.

There is no exception handling. This code may fail in several places. I need to make sure the code is much safer than in its current state

modularise pattern matching classes, e.g., creating a base model for TemporalAnomalyDetector and treat it as an interface for further implementations.

from reddit: https://www.reddit.com/r/AutonomousVehicles/comments/16pmmla/comment/k334nxd/?context=3