Feature/bump hydra instance seg#26
Conversation
…mg even there's no object been detected.
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I'm mostly okay with the instance segmentation changes themselves (though we should plan on spending some time doing some code cleanup sooner rather than later), but will not merge changes that introduce a dependency on ros_system_monitor. Happy to split the PR into two (the instance segmentation and the status monitor changes) if you want to get the yoloe model in sooner, but also shouldn't be too bad to switch to the "json" status monitor variant that I'm already using for the closed-set models
| import spark_config as sc | ||
| import torch | ||
| from rclpy.node import Node | ||
| from ros_system_monitor_msgs.msg import NodeInfoMsg |
There was a problem hiding this comment.
Strongly opposed to requiring ADT4 message definitions in any standalone code; take a look at how I'm doing it for the closed set model
| <depend>tf2_eigen</depend> | ||
| <exec_depend>rclpy</exec_depend> | ||
| <exec_depend>image_transport_plugins</exec_depend> | ||
| <exec_depend>ros_system_monitor_msgs</exec_depend> |
There was a problem hiding this comment.
Strongly opposed to this being a dependency
| msg.notes = f"No inference for {now - self._last_inference_time:.1f}s." | ||
| else: | ||
| msg.status = NodeInfoMsg.NOMINAL | ||
| msg.notes = "Running." |
There was a problem hiding this comment.
(minor) it might be helpful to publish the windowed average of the inference time per frame while this is running
| self.get_logger().debug("No masks detected in the image.") | ||
| return | ||
| # we should still publish image even when detection is None | ||
| instance_seg_img = np.zeros((img.shape[0], img.shape[1])) |
There was a problem hiding this comment.
This might already be handled by how ret.instance_seg_img works (though I forget if this is a property that computes the instance+category ID image from the masks or not), preference is that we push this logic to how the instance segmentation image is calculated instead of here
|
|
||
| self.init_model() | ||
|
|
||
| def init_model(self): |
There was a problem hiding this comment.
In general, it's bad practice to defer to a class method in a constructor, especially when you're using it to get different behavior for a derived class. The way that I'd implement this is to either have a base class that both the Yolov11 and the Yoloe wrapper inherit from (that implements the forward logic, but doesn't init a model), or move the logic for filtering the returned masks, etc somewhere else and not having the yoloe wrapper inherit from the yolov11 wrapper
There was a problem hiding this comment.
Actually, the two models only differ in how they get created, one called YOLO and the other called YOLOE. Do you think it's better to just create a YOLOSeg class that can switch between the model initializer based on the config? Like
if "yoloe" in config.model_name:
# init using YOLOE and set segmentation class
There was a problem hiding this comment.
I think that's viable as an option (it's similar to the second option I suggested), but I think we already have the model class from the virtual config type and it seems a little bit more brittle to get the model class by parsing the weights name. Open to your suggestion, or you could just copy-paste the two classes and not have the YoloE model inherit from Yolo for now. We can sit down and do some cleanup together after we merge this if it ends up being convenient this week
There was a problem hiding this comment.
I think it makes more sense to have a base class now, after I checked out the yolo documentations, especially if we want to try out other models, like yolo world or future yolo models. A base class will come in handy.
Also, I'm now stopping calling yolov11 and switching to yolo-seg becuase we can have various weights now (v11 or v26, and more in the future)
| imgsz=(img.shape[0], img.shape[1]), | ||
| # Set True to maintain original image size for masks, | ||
| # required for hydra, but slow down the model | ||
| retina_masks=True, |
There was a problem hiding this comment.
We can talk about how to fix this if we don't like performance of the masks on the ultralytics side (i.e., should be feasible to rescale and clean up the masks ourselves somewhere else)
…base class. Change yolov11 name to yolo-seg becuase it now supports multiple segmentation version.
2 participants
Add yoloe as a model for segmentation