Skip to content

Conversation

@gardeg
Copy link

@gardeg gardeg commented Oct 26, 2025

unet predict_ros made with launch file and increased readability

@gardeg gardeg requested review from jorgenfj and kluge7 October 26, 2025 13:38
@gardeg gardeg self-assigned this Oct 26, 2025
@gardeg gardeg linked an issue Oct 26, 2025 that may be closed by this pull request
5 tasks
Copy link

@jorgenfj jorgenfj left a comment

Choose a reason for hiding this comment

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

You pass the yaml file to the node in the launch file, but the ros2 param api is never actually used in the node. Instead the node loads the yaml fiel into a python config and uses that instead. This setup is confusing. Also the config path is something that should be controlled by the launch file and not the node.

Comment on lines 21 to 30
input_topic = DeclareLaunchArgument("input_topic", default_value="/image_color")

node = Node(
package=LaunchConfiguration("package"),
executable="interface.py",
name="unet_segmentation_node",
parameters=[LaunchConfiguration("params_file")],
remappings=[
("/image_color", LaunchConfiguration("input_topic")),
# Overlay and mask topics come from YAML; remap here only if needed:

Choose a reason for hiding this comment

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

Having the input_topic be set both by the config file and the launch config can be ambigous. We should only use 1 of these.

<package format="3">
<name>unet_segmentation</name>
<version>0.0.0</version>
<description>YOLO inference on images, publishing detections and annotated outputs.</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Update description

<name>unet_segmentation</name>
<version>0.0.0</version>
<description>YOLO inference on images, publishing detections and annotated outputs.</description>
<maintainer email="[email protected]">kluge7</maintainer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace this with your own email and github username

@kluge7 kluge7 requested a review from Copilot October 26, 2025 18:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a UNet-based image segmentation ROS2 node with improved modularity and launch configuration. The implementation provides real-time segmentation with configurable inference parameters and publishing of both segmentation masks and visual overlays.

Key Changes:

  • UNet model implementation with configurable architecture (simple vs. full, bilinear vs. transposed convolution upsampling)
  • ROS2 node for real-time image segmentation with dynamic resizing and overlay generation
  • Launch file and YAML configuration for deployment flexibility

Reviewed Changes

Copilot reviewed 10 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
unet_segmentation/utils.py Core utilities for model inference, image preprocessing, mask manipulation, and overlay generation
unet_segmentation/unet/unet_parts.py UNet building blocks including double convolution, down/upsampling modules
unet_segmentation/unet/unet_model.py Main UNet architecture with simple/full variants and gradient checkpointing support
unet_segmentation/unet/init.py Package initialization exposing UNet class
interface.py ROS2 node implementing subscription, inference pipeline, and publishing logic
config.py Configuration dataclass with YAML loading and QoS profile setup
package.xml ROS2 package metadata and dependencies
unet_segmentation.launch.py Launch file with configurable parameters and topic remapping
unet_segmentation.yaml Default configuration parameters for model and runtime settings
CMakeLists.txt Build configuration for Python package and resource installation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<package format="3">
<name>unet_segmentation</name>
<version>0.0.0</version>
<description>YOLO inference on images, publishing detections and annotated outputs.</description>
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

The package description references 'YOLO inference' but this package is for UNet segmentation. The description should be updated to accurately reflect the package's purpose, e.g., 'UNet-based image segmentation node for ROS2, publishing segmentation masks and annotated overlays.'

Suggested change
<description>YOLO inference on images, publishing detections and annotated outputs.</description>
<description>UNet-based image segmentation node for ROS2, publishing segmentation masks and annotated overlays.</description>

Copilot uses AI. Check for mistakes.
@kluge7 kluge7 requested review from Copilot and kluge7 November 5, 2025 14:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 9 out of 12 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 109 to 111
logger.warn(f'Missing keys when loading: {missing}')
if unexpected:
logger.warn(f'Unexpected keys when loading: {unexpected}')
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Use logger.warning() instead of logger.warn(). The warn() method is deprecated in Python's logging module and should be replaced with warning().

Suggested change
logger.warn(f'Missing keys when loading: {missing}')
if unexpected:
logger.warn(f'Unexpected keys when loading: {unexpected}')
logger.warning(f'Missing keys when loading: {missing}')
if unexpected:
logger.warning(f'Unexpected keys when loading: {unexpected}')

Copilot uses AI. Check for mistakes.
Comment on lines 109 to 111
logger.warn(f'Missing keys when loading: {missing}')
if unexpected:
logger.warn(f'Unexpected keys when loading: {unexpected}')
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Use logger.warning() instead of logger.warn(). The warn() method is deprecated in Python's logging module and should be replaced with warning().

Suggested change
logger.warn(f'Missing keys when loading: {missing}')
if unexpected:
logger.warn(f'Unexpected keys when loading: {unexpected}')
logger.warning(f'Missing keys when loading: {missing}')
if unexpected:
logger.warning(f'Unexpected keys when loading: {unexpected}')

Copilot uses AI. Check for mistakes.
Comment on lines +68 to +77
self.inc = torch.utils.checkpoint.checkpoint(self.inc)
self.down1 = torch.utils.checkpoint.checkpoint(self.down1)
self.down2 = torch.utils.checkpoint.checkpoint(self.down2)
self.down3 = torch.utils.checkpoint.checkpoint(self.down3)
self.down4 = torch.utils.checkpoint.checkpoint(self.down4)
self.up1 = torch.utils.checkpoint.checkpoint(self.up1)
self.up2 = torch.utils.checkpoint.checkpoint(self.up2)
self.up3 = torch.utils.checkpoint.checkpoint(self.up3)
self.up4 = torch.utils.checkpoint.checkpoint(self.up4)
self.outc = torch.utils.checkpoint.checkpoint(self.outc)
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

This implementation is incorrect. torch.utils.checkpoint.checkpoint is a function that performs checkpointing during forward pass, not a wrapper for modules. Assigning it to module attributes will break the model. Gradient checkpointing should be applied in the forward() method by wrapping module calls with torch.utils.checkpoint.checkpoint(module, inputs), or by using the module's .gradient_checkpointing_enable() method if available.

Copilot uses AI. Check for mistakes.
Comment on lines +149 to +151
resized_pil = ResizeIfLargerKeepAspect(
int(self.resize_width), int(self.resize_height)
)(pil_img)
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Creating a new ResizeIfLargerKeepAspect instance on every image callback is inefficient. This transform should be created once during initialization (similar to self.image_transforms) and reused.

Copilot uses AI. Check for mistakes.
)(pil_img)
resized_w, resized_h = resized_pil.size

image_tensor = self.image_transforms(resized_pil)
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The image is being resized twice: once manually with ResizeIfLargerKeepAspect at lines 149-151, and again within self.image_transforms which also includes ResizeIfLargerKeepAspect (line 77 in utils.py). This redundant operation should be removed - either use only self.image_transforms(pil_img) or apply the resize once and use transforms without the resize step.

Copilot uses AI. Check for mistakes.
resized_pil = ResizeIfLargerKeepAspect(
int(self.resize_width), int(self.resize_height)
)(pil_img)
resized_w, resized_h = resized_pil.size
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Variable resized_w is not used.

Suggested change
resized_w, resized_h = resized_pil.size
# resized_w, resized_h = resized_pil.size # Removed unused variable assignment

Copilot uses AI. Check for mistakes.
resized_pil = ResizeIfLargerKeepAspect(
int(self.resize_width), int(self.resize_height)
)(pil_img)
resized_w, resized_h = resized_pil.size
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Variable resized_h is not used.

Suggested change
resized_w, resized_h = resized_pil.size
resized_w, _ = resized_pil.size

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@kluge7 kluge7 left a comment

Choose a reason for hiding this comment

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

image

Comment on lines +1 to +3
from .unet_model import UNet as UNet

__all__ = ["UNet"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed to run the code?

@@ -0,0 +1,17 @@
/**:
ros__parameters:
model_path: "/home/gard/ros2_ws/src/vortex-deep-learning-pipelines/unet_segmentation/model/unet-simple-320-240-l-5-e10-b16(1).pth"
Copy link
Contributor

Choose a reason for hiding this comment

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

I want relative path

@kluge7 kluge7 self-requested a review November 5, 2025 17:25
@kluge7 kluge7 requested a review from Copilot November 5, 2025 17:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 9 out of 13 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

unet_segmentation/unet_segmentation/unet_segmentation_node.py:63

        self.net = load_unet(
            model_file=str(model_f),
            n_classes=self.classes,
            device=self.device,
            bilinear=bool(self.bilinear),
            simple=bool(self.simple),
            logger=self.get_logger(),
        )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

self.declare_parameter(name, default)

# Bind as attributes
self.model_flie = self.get_parameter('model_file').value
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'flie' to 'file'.

Suggested change
self.model_flie = self.get_parameter('model_file').value
self.model_file = self.get_parameter('model_file').value

Copilot uses AI. Check for mistakes.
self.bridge = CvBridge()

# Validate model path
model_f = Path(self.model_file).expanduser()
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The attribute self.model_file is undefined due to the typo on line 110 (self.model_flie). This will cause an AttributeError at runtime when trying to access the model file path.

Copilot uses AI. Check for mistakes.

# Load network
self.net = load_unet(
model_file=str(model_f),
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The parameter name model_file does not match the function signature in utils.py which expects model_path. This will cause a TypeError at runtime.

Suggested change
model_file=str(model_f),
model_path=str(model_f),

Copilot uses AI. Check for mistakes.
resized_pil = ResizeIfLargerKeepAspect(
int(self.resize_width), int(self.resize_height)
)(pil_img)
resized_w, resized_h = resized_pil.size
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Variable resized_w is not used.

Suggested change
resized_w, resized_h = resized_pil.size

Copilot uses AI. Check for mistakes.
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.

UNet node for image segmentation

4 participants