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 test for image and video CNNs #134

Merged
merged 13 commits into from May 1, 2021
Merged

Add test for image and video CNNs #134

merged 13 commits into from May 1, 2021

Conversation

xianyuanliu
Copy link
Member

Description

Add test for video cnns and image cnns.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.

@xianyuanliu xianyuanliu requested a review from haipinglu May 1, 2021 00:31
@github-actions github-actions bot added this to In progress in v0.1.0 May 1, 2021
@xianyuanliu xianyuanliu changed the title Add test video cnns Add test for image and video CNNs May 1, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2021

Codecov Report

Merging #134 (13e94bd) into main (108cc54) will increase coverage by 26.44%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #134       +/-   ##
===========================================
+ Coverage   24.56%   51.00%   +26.44%     
===========================================
  Files          44       44               
  Lines        4096     4096               
===========================================
+ Hits         1006     2089     +1083     
+ Misses       3090     2007     -1083     
Impacted Files Coverage Δ
kale/embed/image_cnn.py 96.40% <ø> (+96.40%) ⬆️
kale/predict/class_domain_nets.py 89.00% <0.00%> (+89.00%) ⬆️
kale/embed/video_i3d.py 90.32% <0.00%> (+90.32%) ⬆️
kale/embed/video_feature_extractor.py 96.61% <0.00%> (+96.61%) ⬆️
kale/embed/video_selayer.py 97.00% <0.00%> (+97.00%) ⬆️
kale/embed/video_res3d.py 97.38% <0.00%> (+97.38%) ⬆️
kale/embed/video_se_res3d.py 98.64% <0.00%> (+98.64%) ⬆️
kale/embed/video_se_i3d.py 98.69% <0.00%> (+98.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 108cc54...13e94bd. Read the comment docs.

@xianyuanliu xianyuanliu added the tests Tests and coverage label May 1, 2021

assert isinstance(feature_network, dict)

assert isinstance(feature_network, dict)
Copy link
Member

Choose a reason for hiding this comment

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

Duplication with line 21, remove one of them?

INPUT_BATCH_RGB = torch.randn(2, 3, 16, 112, 112)
INPUT_BATCH_FLOW = torch.randn(2, 2, 8, 112, 112)
SE_LAYERS = ["SELayerC", "SELayerT", "SELayerCoC", "SELayerMC", "SELayerMAC", "SELayerCT", "SELayerTC"]
set_seed(36)
Copy link
Member

Choose a reason for hiding this comment

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

I know that this won't fail the tests but it should be better to always set seed before using random functions, i.e., putting it before line 13 will be better, isn't it?


BATCH_SIZE = 2
INPUT_BATCH = torch.randn(BATCH_SIZE, 128)
INPUT_BATCH_LOGITS = torch.randn(BATCH_SIZE, 1024, 1, 1, 1)
Copy link
Member

Choose a reason for hiding this comment

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

I could guess what it is for but adding some light comment about what line 15 is will help users or new comers. This code not only serves the purpose of test coverage, but also can teach others how to write proper tests. Thanks.

Copy link
Member

@haipinglu haipinglu May 1, 2021

Choose a reason for hiding this comment

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

@xianyuanliu I should make it clearer: could you explain a bit INPUT_BATCH_LOGITS vs INPUT_BATCH? Is INPUT_BATCH_LOGITS some synthetic output from previous layers (i.e. output of the I3D)?

Or provide some references about the _LOGITS. It is not as obvious as INPUT_BATCH.

Copy link
Member

Choose a reason for hiding this comment

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

@xianyuanliu I should make it clearer: could you explain a bit INPUT_BATCH_LOGITS vs INPUT_BATCH? Is INPUT_BATCH_LOGITS some synthetic output from previous layers (i.e. output of the I3D)?

Or provide some references about the _LOGITS. It is not as obvious as INPUT_BATCH.

Or maybe changing _LOGITS to _I3D?

Copy link
Member

@haipinglu haipinglu left a comment

Choose a reason for hiding this comment

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

Amazing efforts to have the coverage doubled, from 25% to 51%!!!!!!

Just some minor comments to address before merging.

@xianyuanliu
Copy link
Member Author

Amazing efforts to have the coverage doubled, from 25% to 51%!!!!!!

Just some minor comments to address before merging.

Updated.
Thanks! So happy last night! I didn't believe it. I thought something must be wrong and checked many times! We are closer to our goal now!

@haipinglu haipinglu enabled auto-merge May 1, 2021 09:52
@haipinglu haipinglu merged commit 3d99c06 into main May 1, 2021
v0.1.0 automation moved this from In progress to Done May 1, 2021
@haipinglu haipinglu deleted the test-video_cnn branch May 1, 2021 10:04
@github-actions github-actions bot mentioned this pull request Jun 21, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests and coverage
Projects
No open projects
v0.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants