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

feat: add MoveNet and PoseNet notebooks for experiment #17

Merged
merged 32 commits into from
Aug 16, 2021
Merged

feat: add MoveNet and PoseNet notebooks for experiment #17

merged 32 commits into from
Aug 16, 2021

Conversation

bhavikapanara
Copy link
Contributor

@bhavikapanara bhavikapanara commented Jun 30, 2021

@commit-lint
Copy link

commit-lint bot commented Jun 30, 2021

Contributors

bhavikapanara

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #17 (b84a92d) into main (8a053b9) will increase coverage by 3.22%.
The diff coverage is 92.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   89.06%   92.29%   +3.22%     
==========================================
  Files          10       14       +4     
  Lines         869     1272     +403     
==========================================
+ Hits          774     1174     +400     
- Misses         95       98       +3     
Impacted Files Coverage Δ
fall_prediction.py 100.00% <ø> (ø)
src/pipeline/inference.py 88.00% <ø> (-0.24%) ⬇️
tests/pipeline/test_fall_detect_posenet.py 100.00% <ø> (ø)
src/pipeline/pose_engine.py 65.65% <38.00%> (-23.41%) ⬇️
src/pipeline/pose_base.py 91.83% <91.83%> (ø)
src/pipeline/movenet_model.py 97.05% <97.05%> (ø)
src/pipeline/fall_detect.py 98.62% <100.00%> (+10.09%) ⬆️
src/pipeline/posenet_model.py 100.00% <100.00%> (ø)
tests/pipeline/test_fall_detect_movenet.py 100.00% <100.00%> (ø)
... and 5 more

@ivelin
Copy link

ivelin commented Jul 3, 2021

@bhavikapanara this is an improvement. It shows MoveNet is a superior model in most cases, although in some cases it incorrectly assigns keypoints to non-human objects.

Several items that still need work:

  1. I did not see in the test set the following images from this user reported issue. Please crop the photos and add to the test set. That will help us understand if movenet is more robust in terms of non-human object distractions:
    Implement more test cases based on public and user contributed video and image data of elderly falls #4

  2. Another set of tests we are also missing is multiple people in the same image. Since MoveNet is designed for scenes with one person, it would be good to know whether it focuses on one person in a scene with multiple people or instead it gets confused and scrambles keypoints across multiple people.

  3. Also, please add to the comparison matrix keypoint labels and confidence scores for drawn keypoints. It can be in a json format next or under each image. That would help us understand whether movenet is able to detect with higher confidence level than posetnet.

Thank you!

Copy link

@ivelin ivelin left a comment

Choose a reason for hiding this comment

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

See comments:
#17 (comment)

@bhavikapanara bhavikapanara requested a review from ivelin July 4, 2021 05:00
Copy link

@ivelin ivelin left a comment

Choose a reason for hiding this comment

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

@bhavikapanara this is an improvement. It shows MoveNet is a superior model in most cases, although in some cases it incorrectly assigns keypoints to non-human objects.

Several items that still need work:

I did not see in the test set the following images from this user reported issue. Please crop the photos and add to the test set. That will help us understand if movenet is more robust in terms of non-human object distractions:
Implement more test cases based on public and user contributed video and image data of elderly falls

Another set of tests we are also missing is multiple people in the same image. Since MoveNet is designed for scenes with one person, it would be good to know whether it focuses on one person in a scene with multiple people or instead it gets confused and scrambles keypoints across multiple people .
#5

Also, please add to the comparison matrix keypoint labels and confidence scores for drawn keypoints. It can be in a json format next or under each image. That would help us understand whether movenet is able to detect with higher confidence level than posetnet.

Thank you!

@ivelin ivelin marked this pull request as draft July 5, 2021 18:26
@bhavikapanara bhavikapanara requested a review from ivelin July 8, 2021 17:16
@bhavikapanara
Copy link
Contributor Author

@bhavikapanara this is an improvement. It shows MoveNet is a superior model in most cases, although in some cases it incorrectly assigns keypoints to non-human objects.

Several items that still need work:

I did not see in the test set the following images from this user reported issue. Please crop the photos and add to the test set. That will help us understand if movenet is more robust in terms of non-human object distractions:
Implement more test cases based on public and user contributed video and image data of elderly falls

Another set of tests we are also missing is multiple people in the same image. Since MoveNet is designed for scenes with one person, it would be good to know whether it focuses on one person in a scene with multiple people or instead it gets confused and scrambles keypoints across multiple people .
#5

Also, please add to the comparison matrix keypoint labels and confidence scores for drawn keypoints. It can be in a json format next or under each image. That would help us understand whether movenet is able to detect with higher confidence level than posetnet.

Thank you!

@ivelin I have updated the notebook with added multiple person images in test samples and also add comparison part of two models' key-point confidence scores.

please review it and let me know if there are any changes.

@ivelin
Copy link

ivelin commented Jul 14, 2021

@bhavikapanara now the notebook shows the information we can use to compare the models objectively.

However there seems to be a bug in the keypoint scores shown. There are several cases where the posenet image has no keypoints drawn, yet the scores are shows as all 50%.

posenet         0.500000         0.500000    0.500000    0.500000

Please double check the score formulas and rendering. Thank you.

@bhavikapanara this is an improvement. It shows MoveNet is a superior model in most cases, although in some cases it incorrectly assigns keypoints to non-human objects.
Several items that still need work:
I did not see in the test set the following images from this user reported issue. Please crop the photos and add to the test set. That will help us understand if movenet is more robust in terms of non-human object distractions:
Implement more test cases based on public and user contributed video and image data of elderly falls
Another set of tests we are also missing is multiple people in the same image. Since MoveNet is designed for scenes with one person, it would be good to know whether it focuses on one person in a scene with multiple people or instead it gets confused and scrambles keypoints across multiple people .
#5
Also, please add to the comparison matrix keypoint labels and confidence scores for drawn keypoints. It can be in a json format next or under each image. That would help us understand whether movenet is able to detect with higher confidence level than posetnet.
Thank you!

@ivelin I have updated the notebook with added multiple person images in test samples and also add comparison part of two models' key-point confidence scores.

please review it and let me know if there are any changes.

Copy link

@ivelin ivelin left a comment

Choose a reason for hiding this comment

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

@bhavikapanara please see comment about incorrect keypoint score values.

@bhavikapanara
Copy link
Contributor Author

@bhavikapanara please see comment about incorrect keypoint score values.

@ivelin I have fixed the bug. Can you please check it?

MoveNet_Vs_PoseNet.ipynb Outdated Show resolved Hide resolved
MoveNet_Vs_PoseNet.ipynb Outdated Show resolved Hide resolved
MoveNet_Vs_PoseNet.ipynb Outdated Show resolved Hide resolved
@ivelin
Copy link

ivelin commented Jul 24, 2021

@bhavikapanara some additional evidence that the posenet keypoint score calculation is off - either in the notebook or the current production code. Note that score calculation should be done in one place (a single source of truth - the fall detection high level API) and the notebook should be simply iterating over the available posedetection implementations without reimplementing low level tensor arithmetics.

See screenshots that demonstrate inconsistencies between the originally reported posenet detections and their average confidence score next to the notebook scores that appear to be inconsistent. Moreover the notebook seems to draw keypoints and body lines in different coordinates as compared to the originals.

105122715-56a53e00-5a9c-11eb-9a00-c6d18420a882
Screen Shot 2021-07-24 at 12 06 28 PM

105122713-560ca780-5a9c-11eb-81ba-3d68fbcbef54

Screen Shot 2021-07-24 at 12 07 31 PM

105122714-56a53e00-5a9c-11eb-897b-55a9a2f17102

Screen Shot 2021-07-24 at 12 04 12 PM

Copy link

@ivelin ivelin left a comment

Choose a reason for hiding this comment

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

@bhavikapanara this is a big improvement. A few inline comments plus:

  1. Let's add the inference time in the comparison matrix so we can see how the two models compare given different sample inputs.
  2. Please convert the sample iteration from the notebook matrix into a set of unit tests with matching keypoint scores for each AI model and test image. We will use this as a base line to protect us from future regressions in the code for each AI model.

Thank you!

install_requirements.sh Outdated Show resolved Hide resolved
src/pipeline/pose_engine.py Outdated Show resolved Hide resolved
@bhavikapanara
Copy link
Contributor Author

@bhavikapanara this is a big improvement. A few inline comments plus:

  1. Let's add the inference time in the comparison matrix so we can see how the two models compare given different sample inputs.
  2. Please convert the sample iteration from the notebook matrix into a set of unit tests with matching keypoint scores for each AI model and test image. We will use this as a base line to protect us from future regressions in the code for each AI model.

Thank you!

@ivelin The first point is completed. you can now see inference time in the notebook.
But, I didn't get you on 2nd point. Can you please explain it what I need to do exactly?

@bhavikapanara bhavikapanara marked this pull request as ready for review August 6, 2021 12:32
@ivelin
Copy link

ivelin commented Aug 6, 2021

@bhavikapanara this is a big improvement. A few inline comments plus:

  1. Let's add the inference time in the comparison matrix so we can see how the two models compare given different sample inputs.
  2. Please convert the sample iteration from the notebook matrix into a set of unit tests with matching keypoint scores for each AI model and test image. We will use this as a base line to protect us from future regressions in the code for each AI model.

Thank you!

@ivelin The first point is completed. you can now see inference time in the notebook.
But, I didn't get you on 2nd point. Can you please explain it what I need to do exactly?

@bhavikapanara a couple of comments on the time measurement implementation.

  1. time.time() is not recommended for performance measurement because its granularity is not guaranteed and can be as much as one second. It also does not take into account sleep time. Instead use time.process_time() or timeit.
  2. Your results suggest that movenet is about 2x slower than posenet(with mobilenetv1). That seems inconsistent with your the observations you shared before on our slack channel. Can you comment?

Regarding the second request. It is just an ask to convert the notebook iteration over image samples into unit tests for each of the models. Maybe you can put the image names and expected inference scores in a csv or yaml file that we can expand in the future as we receive additional samples from users. Having this dataset test in our CI will protect the code from regressions.

Copy link

@ivelin ivelin left a comment

Choose a reason for hiding this comment

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

@bhavikapanara very nice work!

See a few inline cleanup comments.

Also, it seems like these two notebooks are outdated and we can remove them to reduce clutter: MoveNet_multiple_images.ipynb
PoseNet-multiple-images.ipynb

src/pipeline/pose_base.py Show resolved Hide resolved
@bhavikapanara
Copy link
Contributor Author

@bhavikapanara this is a big improvement. A few inline comments plus:

  1. Let's add the inference time in the comparison matrix so we can see how the two models compare given different sample inputs.
  2. Please convert the sample iteration from the notebook matrix into a set of unit tests with matching keypoint scores for each AI model and test image. We will use this as a base line to protect us from future regressions in the code for each AI model.

Thank you!

@ivelin The first point is completed. you can now see inference time in the notebook.
But, I didn't get you on 2nd point. Can you please explain it what I need to do exactly?

@bhavikapanara a couple of comments on the time measurement implementation.

  1. time.time() is not recommended for performance measurement because its granularity is not guaranteed and can be as much as one second. It also does not take into account sleep time. Instead use time.process_time() or timeit.
  2. Your results suggest that movenet is about 2x slower than posenet(with mobilenetv1). That seems inconsistent with your the observations you shared before on our slack channel. Can you comment?

Regarding the second request. It is just an ask to convert the notebook iteration over image samples into unit tests for each of the models. Maybe you can put the image names and expected inference scores in a csv or yaml file that we can expand in the future as we receive additional samples from users. Having this dataset test in our CI will protect the code from regressions.

yes, notebook experiments say movenet is about 2x slower than posenet(with mobilenetv1). I am also surprised by it. This one also represents movenet is Ultrafast. Don't know what's the issue.

@ivelin
Copy link

ivelin commented Aug 9, 2021

yes, notebook experiments say movenet is about 2x slower than posenet(with mobilenetv1). I am also surprised by it. This one also represents movenet is Ultrafast. Don't know what's the issue.

OK, good to know we are in sync and the numbers are accurate. The performance numbers in the virtual CI environment are a good relative comparison between models that we can keep track of. We will see how these numbers look on real edge devices.

Please proceed with addressing the other comments in this PR so we can merge.

@bhavikapanara
Copy link
Contributor Author

yes, notebook experiments say movenet is about 2x slower than posenet(with mobilenetv1). I am also surprised by it. This one also represents movenet is Ultrafast. Don't know what's the issue.

OK, good to know we are in sync and the numbers are accurate. The performance numbers in the virtual CI environment are a good relative comparison between models that we can keep track of. We will see how these numbers look on real edge devices.

Please proceed with addressing the other comments in this PR so we can merge.

I think all comments get resolved. Please @ivelin review it and let me know if I missed anything.

@ivelin ivelin merged commit 916271e into ambianic:main Aug 16, 2021
@github-actions
Copy link

github-actions bot commented Oct 3, 2021

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants