Add support for custom classes #71

igonro · 2020-08-18T13:59:31Z

Please, read open-mmlab/mmdetection#2408 for understanding the changes and the justification.

I didn't add the boolean parameter custom_classes (as it is currently in mmdetection), because I didn't know if it would fit or if it would be necessary to do it. Everything else has been done with the intention of following the logic as it is in mmdetection.

CLAassistant · 2020-08-18T13:59:36Z

All committers have signed the CLA.

codecov · 2020-08-18T14:12:03Z

Codecov Report

Merging #71 into master will increase coverage by 0.07%.
The diff coverage is 71.79%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   82.20%   82.28%   +0.07%     
==========================================
  Files          84       84              
  Lines        3974     4013      +39     
  Branches      614      629      +15     
==========================================
+ Hits         3267     3302      +35     
- Misses        576      577       +1     
- Partials      131      134       +3

Flag	Coverage Δ
#unittests	`82.28% <71.79%> (+0.07%)`	⬆️

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

Impacted Files	Coverage Δ
mmseg/datasets/custom.py	`86.79% <69.44%> (-5.08%)`	⬇️
mmseg/datasets/pipelines/loading.py	`97.05% <100.00%> (+0.13%)`	⬆️
mmseg/datasets/cityscapes.py	`21.83% <0.00%> (+1.14%)`	⬆️
mmseg/datasets/pipelines/transforms.py	`97.52% <0.00%> (+1.23%)`	⬆️
mmseg/datasets/ade.py	`100.00% <0.00%> (+12.50%)`	⬆️
mmseg/datasets/voc.py	`100.00% <0.00%> (+20.00%)`	⬆️

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 c3e4dbc...5d0fda4. Read the comment docs.

xvjiarui · 2020-08-19T07:55:12Z

Hi @igonro
Thanks for your contribution.
But in this PR, training on the only subclass is still not supported since the annotation is the same.
We need to modify data pipeline to make it work.

igonro · 2020-08-19T12:42:05Z

Is that functionality already being implemented? If not, I could try to implement it. @xvjiarui

xvjiarui · 2020-08-19T12:43:58Z

Hi @igonro
It not implemented yet. PRs are welcome!

igonro · 2020-08-19T14:36:52Z

Have you identified any problem or any way to proceed to implement it? I'm still a newbie to mmsegmentation, so it would be great to get some hint (if you have thought about it). @xvjiarui

xvjiarui · 2020-08-21T15:02:40Z

Hi @igonro
Sorry for the late reply.
Since the pixel label is determined by the annotation file, we may need to modify the loading annotation pipeline here.
Very naive way to achieve this is to filter and re-assign the gt_semantic_seg. But it is not very neat. Any idea?

igonro · 2020-08-25T14:15:11Z

Hi @xvjiarui, can you check out the last changes I have made?

mmseg/datasets/custom.py

xvjiarui · 2020-08-27T08:40:03Z

mmseg/datasets/custom.py

@@ -160,6 +187,8 @@ def get_ann_info(self, idx):
    def pre_pipeline(self, results):
        """Prepare results dict for pipeline."""
        results['seg_fields'] = []
+        if self.custom_classes:
+            results['old_id_to_new_id'] = self.old_id_to_new_id


Could we use something like label_map and add some comments about it?

I don't understand what you mean by label_map. Do you mean changing the key name?

Sorry for the ambiguity.
Yes, 'old_id_to_new_id' is a bit too long. But 'label_map' may not be straight forward enough. Any idea?

Maybe class_transform, class_id_transform or label_transform?

Or label_map could be good with a comment explaining it.

Then label_map it is. We may add comments about key and value of this dict.

We may also add comments about what's the purpose of adding this dict.

So shall we rename it to label_map?

mmseg/datasets/custom.py

xvjiarui · 2020-08-27T09:17:50Z

mmseg/datasets/custom.py

+                if matched_classes < len(classes):
+                    raise ValueError('classes is not a subset of CLASSES.')


We may check this by python builtin issubset.

mmseg/datasets/pipelines/loading.py

xvjiarui · 2020-08-27T09:21:49Z

mmseg/datasets/custom.py

+                self.old_id_to_new_id = {}
+                matched_classes = 0
+                for i, c in enumerate(self.CLASSES):
+                    if c not in classes:


We may remove this condition by checking issubset first.

xvjiarui · 2020-08-28T14:53:27Z

mmseg/datasets/custom.py

+    def get_palette_for_custom_classes(self, palette=None):
+
+        if palette:
+            if isinstance(palette, str):
+                # take it as a file path
+                palette_values = mmcv.list_from_file(palette)
+            elif isinstance(palette, (tuple, list)):
+                palette_values = palette
+            else:
+                raise ValueError(
+                    f'Unsupported type {type(palette)} of palette.')
+
+        else:
+            palette_values = []
+            for x in sorted(self.old_id_to_new_id.items(), key=lambda x: x[1]):
+                if x[1] != -1:
+                    palette_values.append(self.PALETTE[x[0]])
+
+            palette_values = type(self.PALETTE)(palette_values)
+
+        return palette_values


Custom palette from user input may not be necessary for CustomDataset. We may directly set palette by indices from subclasses.

Yes, sorry. I was still fixing some things... You can now take a look if you can.

xvjiarui · 2020-08-28T15:42:10Z

tests/test_data/test_dataset.py

+        classes=None,
+        test_mode=True)
+
+    assert custom_dataset.CLASSES == original_classes


We may add a test case for LoadAnnotation pipeline when there is custom classes.
To achieve this, we may generate a random ground truth segmentation map.

Should I add it in test_loading.py or in this same file? Also I'm not very sure what type of test I should add.

We may add it in test_loading.py.
We should check if the loaded annotation is of custom classes as desired.

xvjiarui · 2020-08-28T15:43:06Z

You may change the draft status when this PR is ready.

xvjiarui · 2020-09-06T04:06:18Z

mmseg/datasets/custom.py

+        if palette:
+            if isinstance(palette, str):
+                # take it as a file path
+                palette = mmcv.list_from_file(palette)


The palette is a list of list. It is not trivial to load palette from path.
I suggest removing this function. We just need to select the right palette if there is.

igonro · 2020-09-07T11:40:47Z

tests/test_data/test_loading.py

+        test_gt[6:8, 2:4] = 3
+        test_gt[6:8, 6:8] = 4
+
+        img_path = osp.join(osp.dirname(__file__), 'img.jpg')


I've tried using tmpdir and tmp_path for doing this, but it raises an error when calling imwrite (not sure why), so I've had to make this workaround.

igonro · 2020-09-14T07:55:17Z

Hey @xvjiarui, could you review this when you have the time? Thanks!

xvjiarui · 2020-09-14T08:04:53Z

Hi @igonro
Sorry for the late reply.
This comment has not be resolved yet.
Could you resolve it first? I will look into tmpdir issue today.

Update:
tmpdir looks fine.

igonro · 2020-09-14T08:48:26Z

Hi @igonro
Sorry for the late reply.
This comment has not be resolved yet.
Could you resolve it first? I will look into tmpdir issue today.

Fixed. I've missunderstood your comment.

xvjiarui · 2020-09-14T10:11:45Z

mmseg/datasets/custom.py

-                 reduce_zero_label=False):
+                 reduce_zero_label=False,
+                 classes=None,
+                 palette=None):


palette arg is not needed.

xvjiarui · 2020-09-14T10:12:00Z

mmseg/datasets/custom.py

@@ -230,6 +244,49 @@ def get_gt_seg_maps(self):

        return gt_seg_maps

+    def get_classes_and_palette(self, classes=None, palette=None):


palette arg is not needed.

xvjiarui · 2020-09-14T10:12:39Z

mmseg/datasets/custom.py

+                else:
+                    self.label_map[i] = classes.index(c)
+
+        return class_names, palette


We need to return the subset of PALETTE since we are using a subset of CLASSES.

xvjiarui · 2020-09-14T12:22:02Z

Hi @igonro
Sorry for the late reply.
This comment has not be resolved yet.
Could you resolve it first? I will look into tmpdir issue today.

Fixed. I've missunderstood your comment.

Hi @igonro
There are still some uncleaned snippets about palette. Please refer to the comment for detail.

…segmentation into add-support-for-custom-classes

igonro · 2020-09-16T10:53:30Z

Hi @xvjiarui
I made those changes. I hope that now everything is fine.

* Support for custom classes * Fix test * Fix pre-commit * Add pipeline logic for custom classes * Fix minor issues, fix test * Fix issues from PR review * Fix tests * Remove palette as str * Rename old_to_new_ids to label_map * Test for load_anns * Remove get_palette function * fixed temp * Add subset of palette, remove palette as arg * minor update Co-authored-by: Jiarui XU <xvjiarui0826@gmail.com>

* refactor fir up/down sample * remove variance scaling * remove variance scaling from unet sde * refactor Linear * style * actually remove variance scaling * add back upsample_2d, downsample_2d * style * fix FirUpsample2D

…ab#71)

igonro added 3 commits August 18, 2020 10:17

Support for custom classes

06d5562

Fix test

38c617c

Fix pre-commit

538819e

hellock requested a review from xvjiarui August 18, 2020 15:33

igonro added 2 commits August 25, 2020 14:50

Add pipeline logic for custom classes

f0ed77f

Fix minor issues, fix test

993e290