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

Test video domain adapter #145

Merged
merged 15 commits into from May 12, 2021
Merged

Test video domain adapter #145

merged 15 commits into from May 12, 2021

Conversation

xianyuanliu
Copy link
Member

@xianyuanliu xianyuanliu commented May 10, 2021

Description

  1. Create test_video_domain_adapter.
  2. Remove some emails.
  3. Change action_multi_domain to video_multi_domain.
  4. Create test/helpers to save demo models & datasets (not sure if we need it).

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • In-line docstrings updated.
  • Source for documentation at docs manually updated for new API.

@xianyuanliu xianyuanliu added work-in-progress Work in progress that should NOT be merged tests Tests and coverage labels May 10, 2021
@github-actions github-actions bot added this to In progress in v0.1.0 May 10, 2021
@xianyuanliu xianyuanliu removed the request for review from haipinglu May 11, 2021 11:18
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #145 (bc31d02) into main (a2f0347) will increase coverage by 10.90%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #145       +/-   ##
===========================================
+ Coverage   73.18%   84.08%   +10.90%     
===========================================
  Files          44       44               
  Lines        4098     4122       +24     
===========================================
+ Hits         2999     3466      +467     
+ Misses       1099      656      -443     
Impacted Files Coverage Δ
kale/embed/video_feature_extractor.py 96.61% <ø> (ø)
kale/embed/video_i3d.py 90.32% <ø> (ø)
kale/embed/video_res3d.py 97.38% <ø> (ø)
kale/embed/video_se_i3d.py 98.69% <ø> (ø)
kale/embed/video_se_res3d.py 98.64% <ø> (ø)
kale/embed/video_selayer.py 97.00% <ø> (ø)
kale/loaddata/video_multi_domain.py 75.70% <100.00%> (ø)
kale/pipeline/domain_adapter.py 89.10% <100.00%> (ø)
kale/pipeline/video_domain_adapter.py 96.93% <100.00%> (+96.93%) ⬆️
kale/loaddata/multi_domain.py 88.39% <0.00%> (+0.89%) ⬆️
... and 5 more

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 a2f0347...bc31d02. Read the comment docs.

@xianyuanliu xianyuanliu removed the work-in-progress Work in progress that should NOT be merged label May 11, 2021
@@ -19,6 +21,7 @@
@pytest.mark.parametrize("source_name", SOURCES)
@pytest.mark.parametrize("target_name", TARGETS)
def test_get_source_target(source_name, target_name, download_path):
download_path = os.path.join(os.path.dirname(os.path.dirname(os.getcwd())), download_path)
Copy link
Member

@haipinglu haipinglu May 11, 2021

Choose a reason for hiding this comment

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

Why not change download_path in conftest.py this way (of absolute path) instead? You've repeated it three times. Whenever there are many such repetition, think of reduction.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed but some issues raised. I don't know if others use this, so I only change this file. I will try again to change the conftest.py

Copy link
Member

Choose a reason for hiding this comment

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

Why do you make this change in the first place? It works well earlier and you did not change anything relevant to this module.

@@ -0,0 +1,18 @@
# Models for efficient test.
Copy link
Member

Choose a reason for hiding this comment

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

I recall you learn this from somewhere. Add a reference?

@@ -625,39 +659,24 @@ def critic_update_steps(self, batch):
h_s = torch.cat((h_s_rgb, h_s_flow), dim=1)
h_t = torch.cat((h_t_rgb, h_t_flow), dim=1)

# Need to improve to process rgb and flow dividedly in the future.
Copy link
Member

Choose a reason for hiding this comment

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

individually or separately, not dividedly

seed = 36
set_seed(seed)

root_dir = os.path.dirname(os.path.dirname(os.getcwd()))
Copy link
Member

@haipinglu haipinglu May 11, 2021

Choose a reason for hiding this comment

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

Think again, whether this is necessary. If yes, it may be needed in others tests so consider putting in conftest.py.

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.

Finished reviewing. Mostly minor issues, particularly readability of variable names.

@haipinglu haipinglu merged commit 9b9ffcf into main May 12, 2021
@haipinglu haipinglu deleted the test-video-domain-adapter branch May 12, 2021 07:41
v0.1.0 automation moved this from In progress to Done May 12, 2021
@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