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

Refactor tracking #193

Merged
merged 56 commits into from
Jun 28, 2024
Merged

Refactor tracking #193

merged 56 commits into from
Jun 28, 2024

Conversation

nikk-nikaznan
Copy link
Collaborator

@nikk-nikaznan nikk-nikaznan commented Jun 24, 2024

After #141
Another attempt of #106
Probably first attempt towards #190

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 26.26932% with 334 lines in your changes missing coverage. Please review.

Project coverage is 39.60%. Comparing base (e247e89) to head (380da85).

Files Patch % Lines
crabs/tracker/track_video.py 0.00% 99 Missing ⚠️
crabs/tracker/sort.py 0.00% 77 Missing ⚠️
crabs/tracker/utils/sort.py 0.00% 65 Missing ⚠️
crabs/tracker/utils/io.py 0.00% 38 Missing ⚠️
crabs/tracker/utils/tracking.py 61.53% 15 Missing ⚠️
crabs/tracker/evaluate_tracker.py 86.36% 12 Missing ⚠️
crabs/detector/utils/detection.py 23.07% 10 Missing ⚠️
crabs/detector/utils/train.py 40.00% 9 Missing ⚠️
crabs/detector/evaluate_model.py 0.00% 7 Missing ⚠️
crabs/detector/utils/evaluate.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   37.91%   39.60%   +1.68%     
==========================================
  Files          20       24       +4     
  Lines        1440     1414      -26     
==========================================
+ Hits          546      560      +14     
+ Misses        894      854      -40     

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

@nikk-nikaznan nikk-nikaznan requested a review from sfmig June 27, 2024 15:49
@nikk-nikaznan nikk-nikaznan marked this pull request as ready for review June 28, 2024 08:42
@sfmig
Copy link
Collaborator

sfmig commented Jun 28, 2024

Hi @nikk-nikaznan,

I liked this organisation, feels clearer!

I had a go at changing a few things - below an attempt at explaining my thinking. Let me know thoughts.

general

  • I renamed the detection module ---> detector.
  • I renamed the tracking module ---> tracker.

This is to avoid repetition between higher and lower level modules.

detector module

  • I added a crabs.detector.utils submodule.
    • It holds the previous _utils modules.
    • Specifically, I split detection_utils and evaluation_utils into:
      • crabs.detector.utils.detection: utils shared by evaluation and training,
      • crabs.detector.utils.train: utils only used in training,
      • crabs.detector.utils.evaluate: utils only used in evaluation.
    • I also moved the optuna utils there (named optuna for now, but we can discuss!) and the visualization.

Initially I wasn't sure if it would be too much nesting (mainly wanted to try). But I think it leaves the crabs.detector level a bit more clear at a glance. It may help writing unit tests too.

tracker module

In the tracker module I did some more substantial changes. I tried to follow the structure of crabs.detector.

  • I added a crabs.tracker.sort module, which only has the tracker classes
    • (somewhat equivalent to crabs.detector.models).
  • I added a crabs.tracker.evaluate_tracker module with a TrackerEvaluate class
    • (somewhat equivalent to crabs.detector.evaluate_model and the DetectorEvaluate class).
  • I added a crabs.tracker.track_video module which would be the main entry point
    • (somewhat equivalent to crabs.detector.train_model or crabs.detector.evaluate_model).
  • a crabs.tracker.utils submodule, which holds
    • sort utils: I extracted them from the sort script,
    • tracking utils: the previous tracking_utils,
    • io utils. This is the most substantial change. The io module contains what was called before inference, removing the class. The class was called Inference but it seemed mostly io operations (there was no prediction or tracking in the class), so I thought this format could work better.

other

I think having a separate config is a good idea!

I also changed the entry points name for the tracker to something a bit more explicit (albeit longer): detect-and-track-video.

I did my best but I am les familiar with the tracking bits, so there may be fixes required in the tracking 🙈

@nikk-nikaznan
Copy link
Collaborator Author

Thank you Sofia! Looking good. I just fixed some tracking bits that throw error. I am happy with the new name convention, I just tried to follow the module name as in the issue, but have no strong opinion on either. The only thing, can we not rename optuna_util as optuna? We can either do _optuna or hyperparameter_optimizer or hpo?

Copy link
Collaborator

@sfmig sfmig left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

I leave you the satisfaction of pressing the big green 🔘 😁

@nikk-nikaznan nikk-nikaznan merged commit 73d539d into main Jun 28, 2024
6 checks passed
@nikk-nikaznan nikk-nikaznan deleted the nikkna/refactor-tracking branch June 28, 2024 15:36
sfmig added a commit that referenced this pull request Jul 8, 2024
* adding config file, load from checkpoint

* adding inference to toml

* adding bash script

* change variable

* change variable

* naming error

* naming error

* fixed import

* cleaned up sort

* add app_wrapper

* changed accelerator

* bugs

* removed accelerator

* removed accelerator

* wrong path

* edit path

* adding batches

* debugging oom

* save video to false

* save video to false

* adding device

* revert the batch out

* modify bash script

* add guide

* debugging

* fixed codec

* cleaned up

* adding gt_dir

* codev revert

* moved tracking to own folder

* precommit

* changes

* wrong filename

* split inference and evaluation

* config file for tracking

* fixed the test

* rename detector_tracking to detector

* remove the bash script for now

* remove the guide for now

* fixed test

* fixed test and rebase to main

* fixed test

* Suggestion for modules organisation

* fixing some tracking bits

* remove unused line

* Update faster_rcnn.yaml

Signed-off-by: nikk-nikaznan <[email protected]>

* Rename optuna module to hpo

* add typehint

---------

Signed-off-by: nikk-nikaznan <[email protected]>
Co-authored-by: sfmig <[email protected]>
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.

3 participants