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

Add depth module #247

Merged
merged 52 commits into from
Dec 1, 2022
Merged

Add depth module #247

merged 52 commits into from
Dec 1, 2022

Conversation

ejm714
Copy link
Collaborator

@ejm714 ejm714 commented Nov 17, 2022

This implementation takes in a folder of videos (or csv with video filepaths) and outputs a csv of distance estimates for each frame (and detection).

For frames with no detection, distance is null. If there are multiple detections in that frame, there is one line per detection. For example

filepath,time,distance
tests/assets/videos/data/raw/goualougo_2013/gorillas_2013/MPI_FID_20_Duboscia/14-Jun-2013/FID_20_Duboscia_2013-6-14_0083.AVI,0,7.4
tests/assets/videos/data/raw/goualougo_2013/gorillas_2013/MPI_FID_20_Duboscia/14-Jun-2013/FID_20_Duboscia_2013-6-14_0083.AVI,0,7.4
tests/assets/videos/data/raw/goualougo_2013/gorillas_2013/MPI_FID_20_Duboscia/14-Jun-2013/FID_20_Duboscia_2013-6-14_0083.AVI,1,
tests/assets/videos/data/raw/goualougo_2013/gorillas_2013/MPI_FID_20_Duboscia/14-Jun-2013/FID_20_Duboscia_2013-6-14_0083.AVI,2,
tests/assets/videos/data/raw/goualougo_2013/gorillas_2013/MPI_FID_20_Duboscia/14-Jun-2013/FID_20_Duboscia_2013-6-14_0083.AVI,3,

Not included in this PR:

  • Masking out other detections where there is more than 1. Under the current implementation, if there are two detections in the frame, the distance estimate will be the same for both detections (since the same input is used for both). This allows us to capture the number of detections in the frame, but not differential distances.

Notes:

  • In the original implementation, there was a bug where the order of the first two frames were swapped: [-1, -2, 0, 1, 2]. This bug is maintained to match what the model expects

Closes https://github.com/drivendataorg/pjmf-zamba/issues/57

Remaining to do:

  • update docs home page to include depth model (and update CLI output)

@netlify
Copy link

netlify bot commented Nov 17, 2022

Deploy Preview for silly-keller-664934 ready!

Name Link
🔨 Latest commit 56d9ffd
🔍 Latest deploy log https://app.netlify.com/sites/silly-keller-664934/deploys/63880f947eb2990009085418
😎 Deploy Preview https://deploy-preview-247--silly-keller-664934.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #247 (56d9ffd) into master (0b9a901) will increase coverage by 0.5%.
The diff coverage is 91.7%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #247     +/-   ##
========================================
+ Coverage    87.0%   87.6%   +0.5%     
========================================
  Files          28      31      +3     
  Lines        1964    2164    +200     
========================================
+ Hits         1709    1896    +187     
- Misses        255     268     +13     
Impacted Files Coverage Δ
zamba/models/publish_models.py 0.0% <0.0%> (ø)
zamba/cli.py 84.8% <74.1%> (-2.0%) ⬇️
zamba/models/config.py 97.4% <95.0%> (+0.5%) ⬆️
zamba/models/depth_estimation/config.py 96.3% <96.3%> (ø)
zamba/models/depth_estimation/depth_manager.py 99.1% <99.1%> (ø)
zamba/models/densepose/config.py 91.8% <100.0%> (-0.7%) ⬇️
zamba/models/depth_estimation/__init__.py 100.0% <100.0%> (ø)
zamba/pytorch/transforms.py 80.3% <100.0%> (+1.1%) ⬆️
zamba/data/video.py 82.4% <0.0%> (+0.7%) ⬆️

setup.cfg Outdated Show resolved Hide resolved
@ejm714 ejm714 marked this pull request as ready for review November 18, 2022 02:54
@ejm714 ejm714 requested a review from pjbull November 18, 2022 02:54
Copy link
Collaborator Author

@ejm714 ejm714 left a comment

Choose a reason for hiding this comment

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

Comments from synchronous code review

setup.cfg Outdated Show resolved Hide resolved
zamba/models/depth_estimation/config.py Outdated Show resolved Hide resolved
zamba/models/depth_estimation/config.py Outdated Show resolved Hide resolved
zamba/models/depth_estimation/config.py Outdated Show resolved Hide resolved
zamba/models/depth_estimation/config.py Show resolved Hide resolved
zamba/models/depth_estimation/depth_manager.py Outdated Show resolved Hide resolved
zamba/models/depth_estimation/depth_manager.py Outdated Show resolved Hide resolved
tests/test_depth.py Show resolved Hide resolved
tests/test_depth.py Show resolved Hide resolved
Copy link
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

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

Docs seem great—just one thought on a clarifying intro sentence!

docs/docs/models/depth.md Show resolved Hide resolved
@ejm714
Copy link
Collaborator Author

ejm714 commented Nov 30, 2022

All PR comments have been addressed. Checked memory usage with scalene and confirmed our predictions are within 0.1 of the competition code, often times closer to a difference of 0.03. This is likely because the competition code does some writing out and reading in of frames with open cv and also uses albumentations to resize (which uses open cv under the hood). We use torchvision transforms instead. Distances are usually measured in increments of 0.5 so these differences don't seem problematic.

Tests are passing locally so I need to look into why they're failing on github.

@pjbull if you have a moment to give the diff a quick read as a backstop, that'd be great. Here's the link

@ejm714 ejm714 merged commit d576ec6 into master Dec 1, 2022
@ejm714 ejm714 deleted the depth-module branch December 1, 2022 18:36
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