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

3 cocodetection mask implement #36

Merged
merged 7 commits into from
Sep 9, 2021
Merged

Conversation

Johansmm
Copy link
Contributor

@Johansmm Johansmm commented Sep 7, 2021

New features:

  • Include Segementation and append_segmentation attribute/function in Frame class
  • Segementation attribute in Frame: To work with segmentation datasets by binary features maps.
    • Update Mask class Include get_view function to render segmentation
  • Use return_mask propierty of COCODetectionDataset to return a segmentation tensor.
  • Implement stuff_ann_file attribut of COCODetectionDataset to provide the stuff annotation json file (complement the things classes).
  • Integrate stuff classes and thing classes in COCODetectionDataset

@Johansmm Johansmm self-assigned this Sep 7, 2021
@Johansmm Johansmm added aloscene aloscene enhancement New feature or request labels Sep 7, 2021
@@ -16,7 +17,7 @@
from torchvision.datasets.coco import CocoDetection

from alodataset import BaseDataset
from aloscene import BoundingBoxes2D, Frame, Labels
from aloscene import BoundingBoxes2D, Frame, Labels, Panoptic
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if Panoptic is the right class name. Panoptic refer to the task it self (stuff + things) while this tensor actually refer to the data. In imgaug, they refer to this type of class as : Segmentation Maps. Maybe Segmentation instead of Panoptic ? What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, you are right. I will change the name

alodataset/coco_detection_dataset.py Show resolved Hide resolved
frame.append_boxes2d(boxes)

if self.prepare.return_masks:
mask = Panoptic(target["masks"], names=("C", "H", "W"), labels=labels_2d)
Copy link
Contributor

Choose a reason for hiding this comment

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

If my understanding is correct the structure is the following:
Frame
- Boxes 2D
- label
- Panoptic
- label

There is therefore a constraints panoptic.shape[0] must be equal to boxes.shape[0] right ?
Is there any reason why you choose to not use one mask per boxes as you previously mentioned ?

Let's talk about it during the day

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. len(labels) == len(Boxes 2D) and there is a constraints with len(labels) == len(Panoptic). So len(Panoptic) == len(Boxes 2D).

The reason is used Segementation in applications that not required boxes.

level=logging.INFO,
format="[%(asctime)s][%(levelname)s] %(message)s",
datefmt="%d-%m-%y %H:%M:%S",
level=logging.INFO, format="[%(asctime)s][%(levelname)s] %(message)s", datefmt="%d-%m-%y %H:%M:%S",
Copy link
Contributor

Choose a reason for hiding this comment

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

Its getting weird to use logger in this main() method while we don't use it in other main methods in the datasets folder. I would suggest to remove it from here, removing the print, and use the sample as for the others datasets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"""

@staticmethod
def __new__(cls, x: Tensor, labels: Union[dict, Labels] = None, names=("N", "H", "W"), *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

We must probably describe what type of information labels is supposed to store. Before to merge,I would also suggest to add the docstrings in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Panoptic was replaced by update of mask class

@Johansmm
Copy link
Contributor Author

Johansmm commented Sep 8, 2021

@thibo73800 All changes were made in the update of the mask class. Ready for revision.

@@ -142,6 +138,19 @@ def append_disparity(self, disparity, name=None):
"""
self._append_label("disparity", disparity, name)

def append_segmentation(self, segmentation: Mask, name: str = None):
"""Attach a mask to the frame.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace mask by segmentation in the docstring ? Also we can add some information to explained why we rely on the Mask augmented tensor to add segmentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I updated the info as follows:

 Parameters
        ----------
        segmentation: aloscene.Mask
            Mask with size (N,H,W), where N is the features maps, each one for one object.
            Each feature map must be a binary mask. For that, is a type of aloscene.Mask

aloscene/mask.py Outdated
tensor = super().__new__(cls, x, *args, **kwargs)
tensor.add_label("labels", labels, align_dim=["N"], mergeable=True)
tensor.add_property("mask_size", x.shape)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line does not seem great to me. If the frame get resized, the mask will be resized and this property might not be updated properly. Also it is still possible to get the shape of the mask at any time using self.shape

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Property removed.

aloscene/mask.py Outdated
tensor = super().__new__(cls, x, *args, **kwargs)
tensor.add_label("labels", labels, align_dim=["N"], mergeable=True)
tensor.add_property("mask_size", x.shape)
tensor.add_property("is_mask", not isinstance(labels, Labels))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does is_mask make sense for a class with the name Mask ?

The following is weird

mask = Mask()
mask.is_mask # False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I removed this property also

@Johansmm
Copy link
Contributor Author

Johansmm commented Sep 9, 2021

@thibo73800 Ready for review again.

Copy link
Contributor

@thibo73800 thibo73800 left a comment

Choose a reason for hiding this comment

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

Sounds good

@thibo73800 thibo73800 merged commit 04e8881 into master Sep 9, 2021
@Johansmm Johansmm deleted the 3-cocodetection-mask-implement branch October 20, 2021 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aloscene aloscene enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants