MaskDINO ResNet50

[eugene123tw]

Summary

Introduces MaskDINO ResNet50 model:

• Added new MaskDINO modules
• Integrated Detectron2 ResNet backbone.
  • Note: While I plan to use the existing ResNet50, the current detectron2.R50 will remain for now.
• Fixed specific issues in the RT-DETR decoder

How to test

Checklist

• I have added unit tests to cover my changes.​
• I have added integration tests to cover my changes.​
• I have ran e2e tests and there is no issues.
• I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
• I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
• I have linked related issues.

License

• I submit my code changes under the same Apache License that covers the project.
  Feel free to contact the maintainers if that's a concern.
• I have updated the license header for each file (see an example below).

# Copyright (C) 2024 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

[eunwoosh]

Thanks for applying my comments. I added a few comments. Please check it. And could you align all docstring to OTX style and add unit test for all added modules?

Module unit test covered under: tests/unit/algo/instance_segmentation/test_maskdino.py

I think an objective of unit test is to check each component is executed as we expected and one of principles to make good unit test is testing one thing (refer). "tests/unit/algo/instance_segmentation/test_maskdino.py" may run most of code added but It is hard to say that it checks every component works as we expected. If it's too time consuming to implement all unit tests, then how about skipping a code copied and not changed at all but implementing a unit test for code implemented or changed by our side?

[sungchul2]

Thanks for this update, I'd like to request few things:

1. there seems to be a lot of duplicate implementations with the otx implementation. Could you check it and if they can be used instead or updated for your implementation?
2. could you update all docstrings in all files you worked?
3. could you consider to modify your implementations to align it with otx instead of adding them as is?

[eugene123tw]

Thanks for this update, I'd like to request few things:

1. there seems to be a lot of duplicate implementations with the otx implementation. Could you check it and if they can be used instead or updated for your implementation?
2. could you update all docstrings in all files you worked?
3. could you consider to modify your implementations to align it with otx instead of adding them as is?

@sungchul2 Thanks for reviewing! The first and third questions are pretty similar—essentially, there are just too many modules to refactor all at once. My intent with this PR is to make the candidate algorithm compatible with OTX (training, testing, prediction, export, optimizer), rather than solving everything in one go. Testing and refactoring will happen in the next stage.

As for the second question, I’m not sure I fully understand. Are you asking me to update "ALL" the docstrings for MaskDINO? I don’t think it’s necessary unless they’re missing. Afaik, all the docstrings are already in place. Could you clarify?

[sungchul2]

@sungchul2 Thanks for reviewing! The first and third questions are pretty similar—essentially, there are just too many modules to refactor all at once. My intent with this PR is to make the candidate algorithm compatible with OTX (training, testing, prediction, export, optimizer), rather than solving everything in one go. Testing and refactoring will happen in the next stage.

As for the second question, I’m not sure I fully understand. Are you asking me to update "ALL" the docstrings for MaskDINO? I don’t think it’s necessary unless they’re missing. Afaik, all the docstrings are already in place. Could you clarify?

As far as I know, otx is following google style docstrings but I found some parts aren't (of course, I didn't check all parts).
https://github.com/openvinotoolkit/training_extensions/blob/develop/for_developers/contribution_guide.md#docstrings

For examples, it is not following the style.

training_extensions/src/otx/algo/instance_segmentation/utils/box_ops.py

Lines 44 to 51 in 8665486

	def generalized_box_iou(boxes1: Tensor, boxes2: Tensor) -> Tensor:
	"""Generalized IoU from https://giou.stanford.edu/.
	The boxes should be in [x0, y0, x1, y1] format
	Returns a [N, M] pairwise matrix, where N = len(boxes1)
	and M = len(boxes2)
	"""

About arguments, I don't know memory argument's role because there is no description. I think it needs to be updated.

training_extensions/src/otx/algo/instance_segmentation/heads/transformer_decoder/dino_decoder.py

Lines 51 to 61 in 8665486

	def forward(
	self,
	tgt: Tensor,
	memory: Tensor,
	tgt_mask: Tensor,
	memory_key_padding_mask: Tensor,
	refpoints_unsigmoid: Tensor,
	spatial_shapes: Tensor,
	valid_ratios: Tensor,
	) -> list[list[Tensor]]:
	"""Forward pass."""

Some parts are missed.

training_extensions/src/otx/algo/instance_segmentation/layers/batch_norm.py

Lines 66 to 77 in 8665486

	class CNNBlockBase(nn.Module):
	"""A CNN block is assumed to have input channels, output channels and a stride.
	The input and output of `forward()` method must be NCHW tensors.
	The method can perform arbitrary computation but must match the given
	channels and stride specification.
	Attribute:
	in_channels (int):
	out_channels (int):
	stride (int):
	"""

Usually, args in docstrings have type hint, but they aren't. It needs to follow others.

training_extensions/src/otx/algo/instance_segmentation/losses/hungarian_matcher.py

Lines 18 to 27 in 8665486

	def batch_dice_loss(inputs: torch.Tensor, targets: torch.Tensor) -> torch.Tensor:
	"""Compute the DICE loss, similar to generalized IOU for masks.
	Args:
	inputs: A float tensor of arbitrary shape.
	The predictions for each example.
	targets: A float tensor with the same shape as inputs. Stores the binary
	classification label for each element in inputs
	(0 for the negative class and 1 for the positive class).
	"""

I think it is your job to find these things and looking for them is a waste of reviewer's time.
Please let reviewers focus on what they need to focus on, just like you said 😊

[eugene123tw]

@sungchul2 Thanks for reviewing! The first and third questions are pretty similar—essentially, there are just too many modules to refactor all at once. My intent with this PR is to make the candidate algorithm compatible with OTX (training, testing, prediction, export, optimizer), rather than solving everything in one go. Testing and refactoring will happen in the next stage.
As for the second question, I’m not sure I fully understand. Are you asking me to update "ALL" the docstrings for MaskDINO? I don’t think it’s necessary unless they’re missing. Afaik, all the docstrings are already in place. Could you clarify?

As far as I know, otx is following google style docstrings but I found some parts aren't (of course, I didn't check all parts). https://github.com/openvinotoolkit/training_extensions/blob/develop/for_developers/contribution_guide.md#docstrings

For examples, it is not following the style.

training_extensions/src/otx/algo/instance_segmentation/utils/box_ops.py

Lines 44 to 51 in 8665486

	def generalized_box_iou(boxes1: Tensor, boxes2: Tensor) -> Tensor:
	"""Generalized IoU from https://giou.stanford.edu/.
	The boxes should be in [x0, y0, x1, y1] format
	Returns a [N, M] pairwise matrix, where N = len(boxes1)
	and M = len(boxes2)
	"""

About arguments, I don't know memory argument's role because there is no description. I think it needs to be updated.

training_extensions/src/otx/algo/instance_segmentation/heads/transformer_decoder/dino_decoder.py

Lines 51 to 61 in 8665486

	def forward(
	self,
	tgt: Tensor,
	memory: Tensor,
	tgt_mask: Tensor,
	memory_key_padding_mask: Tensor,
	refpoints_unsigmoid: Tensor,
	spatial_shapes: Tensor,
	valid_ratios: Tensor,
	) -> list[list[Tensor]]:
	"""Forward pass."""

Some parts are missed.

training_extensions/src/otx/algo/instance_segmentation/layers/batch_norm.py

Lines 66 to 77 in 8665486

	class CNNBlockBase(nn.Module):
	"""A CNN block is assumed to have input channels, output channels and a stride.
	The input and output of `forward()` method must be NCHW tensors.
	The method can perform arbitrary computation but must match the given
	channels and stride specification.
	Attribute:
	in_channels (int):
	out_channels (int):
	stride (int):
	"""

Usually, args in docstrings have type hint, but they aren't. It needs to follow others.

training_extensions/src/otx/algo/instance_segmentation/losses/hungarian_matcher.py

Lines 18 to 27 in 8665486

	def batch_dice_loss(inputs: torch.Tensor, targets: torch.Tensor) -> torch.Tensor:
	"""Compute the DICE loss, similar to generalized IOU for masks.
	Args:
	inputs: A float tensor of arbitrary shape.
	The predictions for each example.
	targets: A float tensor with the same shape as inputs. Stores the binary
	classification label for each element in inputs
	(0 for the negative class and 1 for the positive class).
	"""

I think it is your job to find these things and looking for them is a waste of reviewer's time. Please let reviewers focus on what they need to focus on, just like you said 😊

@sungchul2 Thanks for the suggestion! I will make sure all docstrings comply with the current style. That said, there are a few areas where I have doubts. A lot of the existing code doesn’t align with the standards you mentioned. While I understand your point, the current state of the repository has inconsistent documentation policies, making it unclear how to fully address your concerns.

For example, in rtdetr_decoder.py, there’s no clear documentation on memory handling, and variable memory_level_start_index isn’t even used.

Similarly, in _build_model, the function lacks complete documentation.

I’m not pointing fingers; it’s just that unclear code and sparse documentation exist throughout the codebase. If we aim to uphold a certain standard, we should apply it uniformly, whether it’s for past, present, or future PRs. The process isn’t perfect right now but progress will come.

[kprokofi]

Thank you for your work. Let's make a refactoring in the next PRs

[sovrasov]

@eunwoosh @sungchul2 thanks for the discussion, let's wait for the next PR to continue there