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

[Improvement] set default batch_size of evaluation and testing to 1 #250

Merged
merged 4 commits into from
Nov 27, 2020

Conversation

dreamerlin
Copy link
Collaborator

This PR sets default batch_size of evaluation and testing as 1 to avoid OOM and manual config changes in most cases, since it used the training batch_size as default when videos_per_gpu=cfg.data.get('videos_per_gpu', 2)

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #250 (3a4aecd) into master (17a6f25) will increase coverage by 3.92%.
The diff coverage is 80.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
+ Coverage   82.67%   86.59%   +3.92%     
==========================================
  Files          95       98       +3     
  Lines        6867     6901      +34     
  Branches     1126     1113      -13     
==========================================
+ Hits         5677     5976     +299     
+ Misses        980      708     -272     
- Partials      210      217       +7     
Flag Coverage Δ
unittests 86.58% <80.84%> (+3.92%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmaction/apis/inference.py 81.81% <ø> (ø)
mmaction/apis/train.py 15.00% <0.00%> (ø)
mmaction/datasets/audio_dataset.py 73.68% <ø> (-10.10%) ⬇️
mmaction/datasets/audio_feature_dataset.py 73.68% <ø> (-10.10%) ⬇️
mmaction/datasets/image_dataset.py 83.33% <ø> (ø)
mmaction/datasets/rawframe_dataset.py 88.05% <ø> (+9.72%) ⬆️
mmaction/datasets/rawvideo_dataset.py 93.75% <ø> (+71.02%) ⬆️
mmaction/datasets/video_dataset.py 66.66% <ø> (-15.88%) ⬇️
mmaction/models/losses/hvu_loss.py 87.50% <ø> (ø)
mmaction/datasets/pipelines/loading.py 89.59% <46.15%> (+3.44%) ⬆️
... and 31 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 17a6f25...68a4168. Read the comment docs.

@innerlee
Copy link
Contributor

what does most cases refers to?

@dreamerlin
Copy link
Collaborator Author

dreamerlin commented Oct 14, 2020

what does most cases refers to?

It means the cfg.data.videos_per_gpu is set for training but too large for testing or evaluation (since the testing and evaluation may need more crops), which will cause OOM, and users may need to manually change the config setting for cfg.data.videos_per_gpu or cfg.data.val_dataloader. So we set the videos_per_gpu to 1 to avoid this case though it is slow. And for multi-batch testing, users can manually change the cfg.data.val_dataloader and cfg.data.test_dataloader according to their hardware environment.

@innerlee
Copy link
Contributor

but videos_per_gpu support val and test

@dreamerlin
Copy link
Collaborator Author

see https://github.com/open-mmlab/mmaction2/blob/master/configs/localization/bmn/bmn_400x100_2x8_9e_activitynet_feature.py#L69

U mean it is better to change the config file by setting the val_dataloader or test_dataloader rather than use 1 batch_size as default?

@innerlee
Copy link
Contributor

but too large for testing or evaluation

sure, this solves the problem, isn't it?

mmaction/apis/train.py Outdated Show resolved Hide resolved
tools/test.py Outdated Show resolved Hide resolved
@dreamerlin
Copy link
Collaborator Author

@kennymckormick @innerlee

tools/test.py Outdated
@@ -52,6 +52,10 @@ def parse_args():
help='override some settings in the used config, the key-value pair '
'in xxx=yyy format will be merged into config file. For example, '
"'--cfg-options model.backbone.depth=18 model.backbone.with_cp=True'")
parser.add_argument(
'--multi-batches',
Copy link
Contributor

Choose a reason for hiding this comment

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

what does multi-batch mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will determine whether to set batch as cfg.data.videos_per_gpu first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But cfg.data.videos_per_gpu may raise OOM, so this args provide a option for users.

Copy link
Contributor

Choose a reason for hiding this comment

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

i see

tools/test.py Outdated
@@ -128,7 +132,8 @@ def main():
# build the dataloader
dataset = build_dataset(cfg.data.test, dict(test_mode=True))
dataloader_setting = dict(
videos_per_gpu=cfg.data.get('videos_per_gpu', 2),
videos_per_gpu=cfg.data.get('videos_per_gpu', 1)
if args.multi_batches else 1,
workers_per_gpu=cfg.data.get('workers_per_gpu', 0),
Copy link
Member

Choose a reason for hiding this comment

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

Better to set the default value of 'workers_per_gpu' as 1 (for better testing speed).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would higher value be better?

@kennymckormick
Copy link
Member

@innerlee This PR is of high priority, old codes have bugs when performing localization testing.

Signed-off-by: lizz <lizz@sensetime.com>
Signed-off-by: lizz <lizz@sensetime.com>
@innerlee
Copy link
Contributor

Notes:

I have changed the defaults to small values.
If one want more control, use val_dataloader=dict(videos_per_gpu=1000) or test_dataloader=dict(videos_per_gpu=1000)

@innerlee innerlee merged commit 52cfdee into open-mmlab:master Nov 27, 2020
@dreamerlin dreamerlin deleted the default branch December 14, 2020 02:10
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