-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[Feature] Support eval concate dataset and add tool to show dataset #833
Conversation
Hi @FreyWang |
Please use pre-commit to fix the lint error. |
Please also use pytest to check the code error or compatibility. |
@@ -15,10 +20,107 @@ class ConcatDataset(_ConcatDataset): | |||
datasets (list[:obj:`Dataset`]): A list of datasets. | |||
""" | |||
|
|||
def __init__(self, datasets): | |||
def __init__(self, datasets, separate_eval=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring for separate_eval
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi, I hava fix the issue and add unittest for it, does I need to submit a new PR or not?
@@ -99,6 +99,9 @@ def __init__(self, | |||
self.label_map = None | |||
self.CLASSES, self.PALETTE = self.get_classes_and_palette( | |||
classes, palette) | |||
if test_mode: | |||
assert self.CLASSES is not None, \ | |||
'`cls.CLASSES` or `classes` should be specified when testing' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this modify leads to failed github CI (checked).
Could you please add some unittests and fix the failed unitsests ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will find time to fix the issue above 😂
mmseg/datasets/dataset_wrappers.py
Outdated
raise NotImplementedError( | ||
'All the datasets should have same types when self.separate_eval=False') | ||
else: | ||
gt_seg_maps = chain(*[dataset.get_gt_seg_maps() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the results are pre_eval
results, we do not need gt_seg_maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the results are
pre_eval
results, we do not need gt_seg_maps.
yes, but if pre_eval = False
when training, it may case error
evaluation = dict(interval=2000, metric='mIoU', pre_eval=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean if the results are pre_eval
results, we do not need gt_seg_maps and set gt_seg_maps=None
.
We only need to collect gt_seg_maps
when the results are eval
results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it 😯
def format_results(self, results, imgfile_prefix, indices=None, **kwargs): | ||
"""format result for every sample of ConcatDataset """ | ||
ret_res = [] | ||
for i, indice in enumerate(indices): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about indices=None
?
Maybe we need handle this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
indices=None
?
Maybe we need handle this case.
you are right, I will fix it
mmseg/datasets/dataset_wrappers.py
Outdated
gt_seg_maps = chain(*[dataset.get_gt_seg_maps() | ||
for dataset in self.datasets]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gt_seg_maps = chain(*[dataset.get_gt_seg_maps() | |
for dataset in self.datasets]) | |
if mmcv.is_list_of(results, np.ndarray) or mmcv.is_list_of( | |
results, str): | |
gt_seg_maps = chain(*[dataset.get_gt_seg_maps() | |
for dataset in self.datasets]) | |
else: | |
gt_seg_maps = None |
Does this work?
Please have a check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,i will
Signed-off-by: FreyWang <wangwxyz@qq.com>
Signed-off-by: FreyWang <wangwxyz@qq.com>
Signed-off-by: FreyWang <wangwxyz@qq.com>
Signed-off-by: FreyWang <wangwxyz@qq.com>
Signed-off-by: FreyWang <wangwxyz@qq.com>
Update in this PR. 发自 网易邮箱大师 ---- 回复的原邮件 ---- 发件人 ***@***.***> 日期 2021年09月02日 21:53 收件人 ***@***.***> 抄送至 ***@***.******@***.***> 主题 Re: [open-mmlab/mmsegmentation] [Feature] Support eval concate dataset and add tool to show dataset (#833) @FreyWang commented on this pull request. In mmseg/datasets/dataset_wrappers.py: > @@ -15,10 +20,107 @@ class ConcatDataset(_ConcatDataset):
datasets (list[:obj:`Dataset`]): A list of datasets.
"""
- def __init__(self, datasets):
+ def __init__(self, datasets, separate_eval=True):
hi, I hava fix the issue and add unittest for it, does I need to submit a new PR or not? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.
|
Done. Additionally, one bug fix have been commited, please review it too. bf48690 |
Codecov Report
@@ Coverage Diff @@
## master #833 +/- ##
==========================================
+ Coverage 88.90% 89.00% +0.10%
==========================================
Files 110 110
Lines 5928 5992 +64
Branches 950 966 +16
==========================================
+ Hits 5270 5333 +63
- Misses 465 466 +1
Partials 193 193
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -112,8 +112,6 @@ def total_intersect_and_union(results, | |||
ndarray: The prediction histogram on all classes. | |||
ndarray: The ground truth histogram on all classes. | |||
""" | |||
num_imgs = len(results) | |||
assert len(list(gt_seg_maps)) == num_imgs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove these assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list(gt_seg_maps)
will loop the generator and then gt_seg_maps
will be empty,
for result, gt_seg_map in zip(results, gt_seg_maps): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the assert is not remove,
eval_results = train_dataset.evaluate(pseudo_results, metric=['mIoU']) |
Could you please fix the lint error and add more unittests to improve the coverage? |
OK, I will check again. Actually I did use pre-commit to refactor the code😕 |
mmseg/datasets/builder.py
Outdated
@@ -30,6 +30,7 @@ def _concat_dataset(cfg, default_args=None): | |||
img_dir = cfg['img_dir'] | |||
ann_dir = cfg.get('ann_dir', None) | |||
split = cfg.get('split', None) | |||
separate_eval = cfg.get('separate_eval', True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may pop separate_eval
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me check😢
Please fix the lint |
Signed-off-by: FreyWang <wangwxyz@qq.com>
Signed-off-by: FreyWang <wangwxyz@qq.com> # Conflicts: # tools/test.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please fix the lint error and add more unittests to improve the coverage?
Done
mmseg/datasets/builder.py
Outdated
@@ -49,6 +50,9 @@ def _concat_dataset(cfg, default_args=None): | |||
datasets = [] | |||
for i in range(num_dset): | |||
data_cfg = copy.deepcopy(cfg) | |||
# pop 'separate_eval' since it is not a valid key for common datasets. | |||
if 'separate_eval' in data_cfg: | |||
data_cfg.pop('separate_eval') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate_eval
has been poped here for every subset @xvjiarui
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use separate_eval = cfg.pop('separate_eval', True)
in L33?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use
separate_eval = cfg.pop('separate_eval', True)
in L33?
Sure, I think it will be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 28e3bd2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your efforts, LGTM.
Hi @xvjiarui |
…pen-mmlab#833) * [Feature] Add tool to show origin or augmented train data * [Feature] Support eval concate dataset * Add docstring and modify evaluate of concate dataset Signed-off-by: FreyWang <wangwxyz@qq.com> * format concat dataset in subfolder of imgfile_prefix Signed-off-by: FreyWang <wangwxyz@qq.com> * add unittest of concate dataset Signed-off-by: FreyWang <wangwxyz@qq.com> * update unittest for eval dataset with CLASSES is None Signed-off-by: FreyWang <wangwxyz@qq.com> * [FIX] bug of generator, which lead metric to nan when pre_eval=False Signed-off-by: FreyWang <wangwxyz@qq.com> * format code Signed-off-by: FreyWang <wangwxyz@qq.com> * add more unittest * add more unittest * optim concat dataset builder
Patch Release: 0.5.1
freywang,您好!您在MMSeg项目中给我们提的PR非常重要,感谢您付出私人时间帮助改进开源项目,相信很多开发者会从你的PR中受益。 |
* correct tpn sthv1 testing * Update tpn_tsm_r50_1x1x8_150e_sthv1_rgb.py
This pr
pre_eval=False
Add tool to show origin train set and augmented train set
Add file
tools/browse_dataset.py
,ConcatDataset
andRepeatDataset
is also supported.usage
python tools/browse_dataset.py {CONFIG}
it will save augmented train image to
args.output-dir
python tools/browse_dataset.py {CONFIG} --show-origin
it will save origin train image to
args.output-dir
Support eval concate dataset
this version is compatible with progressive eval #PR709
usage
separate_eval=True
, it will eval every sub dataset separately, else eval as a whole dataset.CityscapesDataset
is not support in concat datasetModification
mmseg/datasets/custom.py
self.CLASSES is not None
in test mode to avoid call generatorgt_seg_maps
repeatedlymmseg/datasets/dataset_wrapper.py
evaluate()
to ConcatDataset. Whenseparate_eval=True
, each subset is evaluated separately. Whenseparate_eval=False
, the generatorgt_seg_maps
of each subset is merged to calculate whole result.pre_eval()
to be compatible with progressive evalformat_results()
to save each result, all image result from different subset will be saved toimgfile_prefix/{dataset_idx}
Some numerical results
Use
configs/fcn/fcn_r50-d8_512x1024_40k_cityscapes.py
and its trained checkpoint as demo, when repeat test set twiceseparate_eval=True
It will eval twice, each 2000 image result is 35.94
sub set 1:
sub set 2:
separate_eval=False
It will eval 2000*2 image as a whole, the result is alse 35.94