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

368 visionnode and segmentation #404

Merged
merged 13 commits into from
Nov 4, 2024
Merged

Conversation

RoyaLxPole
Copy link
Collaborator

@RoyaLxPole RoyaLxPole commented Oct 31, 2024

Description

Research about the segmentation part in the visionnode

Fixes #368

Type of change

Create a new .md file where the research is summarized

Does this PR introduce a breaking change?

no

Most important changes

add new file to /doc/research/paf24/

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (might be obsolete with CI later on)
  • New and existing unit tests pass locally with my changes (might be obsolete with CI later on)

Summary by CodeRabbit

  • New Features
    • Introduced a comprehensive documentation file on segmentation solutions within Autoware.
    • Included sections covering existing solutions, dropped implementations, and Carla sensor integration.
    • Provided code snippets and resource links for setting up segmentation sensors in the Carla simulator.
    • Added images illustrating the semantic segmentation camera setup.
    • Raised a follow-up question regarding the preference for bounding boxes over segmentation models.

@RoyaLxPole RoyaLxPole linked an issue Oct 31, 2024 that may be closed by this pull request
Copy link
Contributor

coderabbitai bot commented Oct 31, 2024

Walkthrough

The changes introduce a new markdown file, segmentation.md, located in doc/research/paf24/. This document outlines research related to the segmentation component of Autoware, featuring sections on already implemented solutions, those that were implemented but later dropped, details about Carla sensors, and a follow-up question regarding bounding boxes versus segmentation models. The document includes links to relevant resources and code snippets for setting up a camera sensor in the Carla simulator.

Changes

File Change Summary
doc/research/paf24/segmentation.md New document added detailing segmentation research, including sections on solutions, Carla sensors, and code snippets.

Assessment against linked issues

Objective Addressed Explanation
Does Carla output segmentations? (368)
Are there already trained models? (368)
If yes, are these models usable? (368) The document mentions models but does not clarify usability.

🐰 In the land of code where rabbits hop,
A new doc blooms, with knowledge on top.
Segmentation secrets and sensors in play,
With links and snippets to brighten the way.
So let’s gather round, and learn with delight,
For in this new file, our future is bright! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
doc/research/paf24/segmentation.md (3)

11-17: Enhance documentation with proper context and formatting.

The section needs improvement in the following areas:

  1. Add context explaining what these solutions are and how they work
  2. Format links with descriptive text instead of bare URLs
  3. Explain the relationship between the implementation and the dataset generator

Example improvement:

## Already implemented solutions

The project currently uses a segmentation model that processes camera input for detecting road features. The model implementation can be found in the [perception launch configuration](https://github.com/una-auxme/paf/blob/c3011ee70039e199e106c54aa162a8f52be241a6/code/perception/launch/perception.launch?plain=1#L59-L61).

The model was trained using a custom dataset generated through our [dataset generator](https://github.com/una-auxme/paf/blob/8e8f9a1a03ae09d5ac763c1a11b398fc1ce144b0/code/perception/src/dataset_generator.py#L109-L110), which processes raw sensor data to create training samples.
🧰 Tools
🪛 Markdownlint

13-13: null
Bare URL used

(MD034, no-bare-urls)


16-16: null
Bare URL used

(MD034, no-bare-urls)


18-20: Document the rationale for dropping EfficientPS.

Please provide context about:

  1. Why EfficientPS was initially chosen
  2. What issues or limitations were encountered
  3. The decision-making process that led to dropping it

This information is crucial for future reference and helps prevent re-investigating already explored solutions.

🧰 Tools
🪛 Markdownlint

18-18: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


18-18: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


19-19: null
Bare URL used

(MD034, no-bare-urls)


40-41: Address architectural decision and fix formatting.

The question raises important architectural considerations that should be addressed explicitly.

Please:

  1. Document the trade-offs between bounding boxes and segmentation models:
    • Performance characteristics
    • Accuracy comparisons
    • Resource requirements
  2. Include benchmarks if available
  3. Document the reasoning behind the final decision

Also, fix the formatting issues:

-## Follow up Question:
-Why the last group used bounding boxes and not the segmentation model is it to slow or maybe not reliable?
+## Follow-up Question
+
+Why did the previous group choose bounding boxes over the segmentation model? Is it too slow or perhaps not reliable enough?
🧰 Tools
🪛 LanguageTool

[grammar] ~40-~40: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...n walks will be labeld as roadlines ## Follow up Question: Why the last group used bound...

(CA_FOLLOW_UP)


[uncategorized] ~41-~41: Possible missing comma found.
Context: ...bounding boxes and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~41-~41: “to” seems less likely than “too” (as well, very, ‘too much’).
Context: ...es and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_CP_TO_TOO)

🪛 Markdownlint

40-40: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


40-40: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8c968fb and 3ad9f53.

⛔ Files ignored due to path filters (1)
  • doc/assets/perception/Carla_Segmentation_Sensor.png is excluded by !**/*.png
📒 Files selected for processing (1)
  • doc/research/paf24/segmentation.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/research/paf24/segmentation.md

[grammar] ~9-~9: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ... Carla Sensors - Follow up Question ## Al...

(CA_FOLLOW_UP)


[grammar] ~40-~40: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...n walks will be labeld as roadlines ## Follow up Question: Why the last group used bound...

(CA_FOLLOW_UP)


[uncategorized] ~41-~41: Possible missing comma found.
Context: ...bounding boxes and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~41-~41: “to” seems less likely than “too” (as well, very, ‘too much’).
Context: ...es and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_CP_TO_TOO)

🪛 Markdownlint
doc/research/paf24/segmentation.md

18-18: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


21-21: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


40-40: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


18-18: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


21-21: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


40-40: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


23-23: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


31-31: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


13-13: null
Bare URL used

(MD034, no-bare-urls)


16-16: null
Bare URL used

(MD034, no-bare-urls)


19-19: null
Bare URL used

(MD034, no-bare-urls)


22-22: null
Bare URL used

(MD034, no-bare-urls)


35-35: null
Bare URL used

(MD034, no-bare-urls)


23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Comment on lines 21 to 39
## Carla Sensors:
https://carla.readthedocs.io/en/0.8.4/cameras_and_sensors/
```
camera = carla.sensor.Camera('MyCamera', PostProcessing='SemanticSegmentation')
camera.set(FOV=90.0)
camera.set_image_size(800, 600)
camera.set_position(x=0.30, y=0, z=1.30)
camera.set_rotation(pitch=0, yaw=0, roll=0)

carla_settings.add_sensor(camera)
```
![Alt text](https://github.com/una-auxme/paf/blob/368-visionnode-and-segmentation/doc/assets/perception/Carla_Segmentation_Sensor.png)

there is another solution implemented by the carla simulator:
https://carla.readthedocs.io/en/0.9.14/ref_sensors/#semantic-segmentation-camera:~:text=the%20object%20it.-,Semantic%20segmentation%20camera,-Blueprint%3A%20sensor

for more context:
the pedestrian walks will be labeld as roadlines

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update Carla sensor documentation and code example.

  1. The code example and documentation link are from Carla 0.8.4, which is outdated
  2. The image link should use a relative path
  3. There's a typo in "labeld" (line 38)

Update the code example to match current Carla version (0.9.14):

# Set up semantic segmentation camera
camera_bp = world.get_blueprint_library().find('sensor.camera.semantic_segmentation')
camera_bp.set_attribute('image_size_x', '800')
camera_bp.set_attribute('image_size_y', '600')
camera_bp.set_attribute('fov', '90.0')

# Add camera to vehicle
camera_transform = carla.Transform(carla.Location(x=0.30, y=0.0, z=1.30))
camera = world.spawn_actor(camera_bp, camera_transform, attach_to=vehicle)

Also, update the documentation link to the current version:

-https://carla.readthedocs.io/en/0.8.4/cameras_and_sensors/
+https://carla.readthedocs.io/en/0.9.14/ref_sensors/#semantic-segmentation-camera

And use relative path for the image:

-![Alt text](https://github.com/una-auxme/paf/blob/368-visionnode-and-segmentation/doc/assets/perception/Carla_Segmentation_Sensor.png)
+![Carla Segmentation Sensor](../../assets/perception/Carla_Segmentation_Sensor.png)
🧰 Tools
🪛 Markdownlint

21-21: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


21-21: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


23-23: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


31-31: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


22-22: null
Bare URL used

(MD034, no-bare-urls)


35-35: null
Bare URL used

(MD034, no-bare-urls)


23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Comment on lines 5 to 9
- [Segmentation](#Segmentation)
- [Already implemended solutions](#1.-Already-implemented-solutions)
- [Implemented but dropped](#2Implemented-but-dropped)
- [Carla Sensors](#3-Carla-Sensors)
- [Follow up Question](#4-Follow-up-Question)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix broken table of contents links.

The anchor links in the table of contents don't match the actual heading formats, which will result in broken navigation.

Apply this diff to fix the navigation:

-- [Already implemended solutions](#1.-Already-implemented-solutions)
-- [Implemented but dropped](#2Implemented-but-dropped)
-- [Carla Sensors](#3-Carla-Sensors)
-- [Follow up Question](#4-Follow-up-Question)
+- [Already implemented solutions](#already-implemented-solutions)
+- [Implemented but dropped](#implemented-but-dropped)
+- [Carla Sensors](#carla-sensors)
+- [Follow-up Question](#follow-up-question)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- [Segmentation](#Segmentation)
- [Already implemended solutions](#1.-Already-implemented-solutions)
- [Implemented but dropped](#2Implemented-but-dropped)
- [Carla Sensors](#3-Carla-Sensors)
- [Follow up Question](#4-Follow-up-Question)
- [Segmentation](#Segmentation)
- [Already implemented solutions](#already-implemented-solutions)
- [Implemented but dropped](#implemented-but-dropped)
- [Carla Sensors](#carla-sensors)
- [Follow-up Question](#follow-up-question)
🧰 Tools
🪛 LanguageTool

[grammar] ~9-~9: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ... Carla Sensors - Follow up Question ## Al...

(CA_FOLLOW_UP)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
doc/research/paf24/segmentation.md (4)

13-16: Enhance documentation of implemented solutions.

The section would be more helpful with:

  1. Brief descriptions of each implemented solution
  2. Properly formatted markdown links instead of raw URLs
  3. Explanation of how the dataset was generated and used

Example format:

## Already implemented solutions

### Current Implementation
The current perception system uses [semantic segmentation launch configuration](link) which was trained using a [custom dataset generator](link).

Key features:
- Description of what the implementation does
- How the dataset was generated
- Performance characteristics
🧰 Tools
🪛 Markdownlint

13-13: null
Bare URL used

(MD034, no-bare-urls)


16-16: null
Bare URL used

(MD034, no-bare-urls)


18-20: Document reasons for dropping EfficientPS implementation.

Please provide context about:

  1. Why was EfficientPS initially chosen?
  2. What issues or limitations led to dropping it?
  3. Lessons learned from this implementation attempt

This information would be valuable for future decision-making.

🧰 Tools
🪛 Markdownlint

18-18: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


18-18: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


19-19: null
Bare URL used

(MD034, no-bare-urls)


29-45: Improve code block formatting and documentation.

  1. Specify the language for the code block (python)
  2. Add comments explaining the key parameters:
    • Image size choice (1920x1080)
    • FOV setting (105)
    • Camera position and rotation values
  3. Document the CityScapesPalette color mapping
-```
+```python
🧰 Tools
🪛 Markdownlint

29-29: Expected: 0 or 2; Actual: 1
Trailing spaces

(MD009, no-trailing-spaces)


29-29: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


45-45: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


29-29: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


51-52: Expand the architectural decision discussion.

This is an important question that deserves thorough investigation. Consider:

  1. Benchmarking both approaches (bounding boxes vs segmentation)
  2. Documenting performance metrics
  3. Analyzing trade-offs in terms of:
    • Processing speed
    • Accuracy
    • Resource usage
    • Implementation complexity

Would you like help creating a comparison framework to evaluate both approaches?

🧰 Tools
🪛 LanguageTool

[grammar] ~51-~51: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...mera,-The%20semantic%20segmentation ## Follow up Question: Why the last group used bound...

(CA_FOLLOW_UP)

🪛 Markdownlint

51-51: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


51-51: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3ad9f53 and 22c8434.

📒 Files selected for processing (1)
  • doc/research/paf24/segmentation.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/research/paf24/segmentation.md

[grammar] ~51-~51: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...mera,-The%20semantic%20segmentation ## Follow up Question: Why the last group used bound...

(CA_FOLLOW_UP)

🪛 Markdownlint
doc/research/paf24/segmentation.md

29-29: Expected: 0 or 2; Actual: 1
Trailing spaces

(MD009, no-trailing-spaces)


18-18: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


21-21: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


51-51: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)


18-18: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


21-21: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


51-51: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


29-29: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


45-45: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


13-13: null
Bare URL used

(MD034, no-bare-urls)


16-16: null
Bare URL used

(MD034, no-bare-urls)


19-19: null
Bare URL used

(MD034, no-bare-urls)


47-47: null
Bare URL used

(MD034, no-bare-urls)


49-49: null
Bare URL used

(MD034, no-bare-urls)


29-29: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (1)
doc/research/paf24/segmentation.md (1)

25-27: Clarify pedestrian crossing labeling details.

The note about pedestrian walks being labeled as roadlines needs more context:

  1. Is this a Carla limitation or a design choice?
  2. What are the implications for detection accuracy?
  3. Are there any workarounds or handling strategies?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
doc/research/paf24/segmentation.md (4)

12-18: Enhance documentation clarity in "Already implemented solutions" section.

The section needs more context and explanation:

  1. Add descriptions of what these solutions actually implement
  2. Explain how the dataset generator relates to the implementation
  3. Use proper markdown link syntax instead of bare URLs

Example improvement:

## Already implemented solutions

The perception stack currently implements segmentation using [this approach](link). This implementation relies on a model that was trained using our [custom dataset generator](link), which creates labeled data for the following classes:
- [list the classes]

The training process and model architecture are documented in...
🧰 Tools
🪛 Markdownlint

14-14: null
Bare URL used

(MD034, no-bare-urls)


17-17: null
Bare URL used

(MD034, no-bare-urls)


19-22: Document reasons for dropping EfficientPS implementation.

To prevent future teams from repeating the same exploration, please document:

  1. Why was EfficientPS considered initially?
  2. What issues or limitations were encountered?
  3. What were the key factors in the decision to drop it?
🧰 Tools
🪛 Markdownlint

19-19: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


21-21: null
Bare URL used

(MD034, no-bare-urls)


33-50: Improve code example documentation and formatting.

  1. Add Python language specification to the code block
  2. Document the key parameters:
    • Image size choice (1920x1080)
    • FOV value (105)
    • Camera position and rotation
  3. Explain the CityScapesPalette color converter
-```
+```python
# Camera configuration for semantic segmentation
# Resolution: 1920x1080 (Full HD) for detailed segmentation
# FOV: 105 degrees to match our requirements
# Position: 2m forward, 1m up for optimal view
🧰 Tools
🪛 Markdownlint

33-33: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


57-59: Improve follow-up question clarity and grammar.

The question raises an important architectural consideration but needs revision:

-## Follow up Question:
-
-Why the last group used bounding boxes and not the segmentation model is it to slow or maybe not reliable?
+## Follow-up Question:
+
+Why did the previous team choose bounding boxes over segmentation models? Was it due to performance concerns or reliability issues?

Consider adding:

  1. Performance benchmarks comparing both approaches
  2. Reliability metrics if available
  3. Trade-offs between the two approaches
🧰 Tools
🪛 LanguageTool

[grammar] ~57-~57: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...mera,-The%20semantic%20segmentation ## Follow up Question: Why the last group used boun...

(CA_FOLLOW_UP)


[uncategorized] ~59-~59: Possible missing comma found.
Context: ...bounding boxes and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~59-~59: “to” seems less likely than “too” (as well, very, ‘too much’).
Context: ...es and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_CP_TO_TOO)

🪛 Markdownlint

57-57: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 22c8434 and 8bfb710.

📒 Files selected for processing (1)
  • doc/research/paf24/segmentation.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/research/paf24/segmentation.md

[grammar] ~57-~57: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...mera,-The%20semantic%20segmentation ## Follow up Question: Why the last group used boun...

(CA_FOLLOW_UP)


[uncategorized] ~59-~59: Possible missing comma found.
Context: ...bounding boxes and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~59-~59: “to” seems less likely than “too” (as well, very, ‘too much’).
Context: ...es and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_CP_TO_TOO)

🪛 Markdownlint
doc/research/paf24/segmentation.md

19-19: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


23-23: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


57-57: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


14-14: null
Bare URL used

(MD034, no-bare-urls)


17-17: null
Bare URL used

(MD034, no-bare-urls)


21-21: null
Bare URL used

(MD034, no-bare-urls)


53-53: null
Bare URL used

(MD034, no-bare-urls)


55-55: null
Bare URL used

(MD034, no-bare-urls)


33-33: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (1)
doc/research/paf24/segmentation.md (1)

28-30: Clarify pedestrian crossing labeling information.

The statement about pedestrian walks being labeled as roadlines needs more context:

  1. Is this a limitation of Carla's segmentation?
  2. What implications does this have for our use case?
  3. Are there any workarounds available?
✅ Verification successful

Pedestrian crossing labeling is a known CARLA semantic segmentation limitation

Based on the documentation links and code context found, this is a known limitation of CARLA's semantic segmentation camera. The statement is documenting a technical constraint where pedestrian crossings/walks are labeled as part of the road lines in CARLA's segmentation system. This is important information for developers to be aware of when working with the segmentation data.

  • The code shows proper setup of CARLA's semantic segmentation camera using the CityScapesPalette
  • The limitation is documented alongside the camera configuration code
  • This behavior is consistent with CARLA's semantic segmentation implementation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing handling of this limitation in the codebase
rg -i "pedestrian.*cross(ing|walk)" --type py

Length of output: 47


Script:

#!/bin/bash
# Let's broaden the search to find any mentions of pedestrian or road line segmentation
rg -i "(pedestrian|roadline|segmentation)" doc/research/paf24/ -A 3 -B 3

Length of output: 6333


Script:

#!/bin/bash
# Also check for any CARLA segmentation documentation or configuration files
fd -e py -e md -e json -e yaml -e yml . | xargs rg -i "(carla.*segment|segment.*carla)" -A 3 -B 3

Length of output: 4133

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
doc/research/paf24/segmentation.md (2)

1-11: Add blank lines around headings for better readability.

The document structure is good, but some headings need proper spacing for better readability.

Apply this diff:

 # Segmentation

 **Summary:** This page contains the research into the segmentation component of Autoware.

 - [Segmentation](#Segmentation)
   - [Already implemented solutions](#already-implemented-solutions)
   - [Implemented but dropped](#implemented-but-dropped)
   - [Carla Sensors](#carla-sensors)
   - [Follow-up Question](#follow-up-question)

23-56: Improve Carla sensor documentation presentation.

  1. Use relative paths for images
  2. Add language identifier to code block
  3. Simplify documentation links
  4. Add explanation about the CityScapesPalette color mapping

Apply these changes:

-![Alt text](https://carla.readthedocs.io/en/0.9.14/img/ref_sensors_semantic.jpg)
-![Alt text](https://carla.readthedocs.io/en/0.9.14/img/tuto_sem.jpg)
+![Semantic Segmentation Sensor](../../assets/carla/ref_sensors_semantic.jpg)
+![Segmentation Example](../../assets/carla/tuto_sem.jpg)

 example:

-```Python
+```python
 # --------------
 # Add a new semantic segmentation camera to my ego
 # --------------

Also, simplify the documentation links:

-<https://carla.readthedocs.io/en/0.9.14/ref_sensors/#semantic-segmentation-camera:~:text=the%20object%20it.-,Semantic%20segmentation%20camera,-Blueprint%3A%20sensor>
+[Carla Semantic Segmentation Camera Documentation](https://carla.readthedocs.io/en/0.9.14/ref_sensors/#semantic-segmentation-camera)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8bfb710 and 5cbcd68.

📒 Files selected for processing (1)
  • doc/research/paf24/segmentation.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/research/paf24/segmentation.md

[grammar] ~57-~57: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...era,-The%20semantic%20segmentation> ## Follow up Question Why the last group used bound...

(CA_FOLLOW_UP)


[uncategorized] ~59-~59: Possible missing comma found.
Context: ...bounding boxes and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~59-~59: “to” seems less likely than “too” (as well, very, ‘too much’).
Context: ...es and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_CP_TO_TOO)

🔇 Additional comments (1)
doc/research/paf24/segmentation.md (1)

12-22: ⚠️ Potential issue

Enhance documentation with more context and explicit findings.

The research documentation should explicitly address the questions from issue #368:

  1. Does Carla output segmentation data for:
    • Lines
    • Street signs
    • Pedestrian crosswalks
  2. Are there pre-trained models available?

Please expand these sections with:

  • Explanations of the linked code and its relevance
  • Why certain solutions were dropped
  • Clear answers to the original research questions

Let's verify the linked code still exists:

✅ Verification successful

Review comment is appropriate and the linked code exists

Based on the verification results:

  1. The linked code exists and is accessible
  2. The code confirms that:
    • Carla does output segmentation data through its instance segmentation camera (verified in dataset_generator.py)
    • The perception.launch file shows active image segmentation capabilities with models like "deeplabv3_resnet101" and "yolov8x-seg"
    • EfficientPS was previously used but was dropped after paf22 (confirmed in efficientps.md)
  3. The documentation structure matches the research questions from issue VisionNode and Segmentation #368

The request for more context and explicit findings is valid because:

  • The current documentation doesn't clearly answer whether specific features (lines, street signs, crosswalks) are included in the segmentation output
  • The pre-trained model availability isn't explicitly addressed
  • The reasons for dropping EfficientPS aren't explained
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the referenced code still exists
gh api repos/una-auxme/paf/commits/c3011ee70039e199e106c54aa162a8f52be241a6 > /dev/null
gh api repos/una-auxme/paf/commits/8e8f9a1a03ae09d5ac763c1a11b398fc1ce144b0 > /dev/null
gh api repos/una-auxme/paf/commits/8c968fb5c6c44c15b2733c5a181c496eb9b244be > /dev/null

Length of output: 233


Script:

#!/bin/bash
# Check if the referenced files exist and show their content
echo "=== Checking perception.launch ==="
fd "perception.launch" --exec cat {} || echo "File not found"

echo -e "\n=== Checking dataset_generator.py ==="
fd "dataset_generator.py" --exec cat {} || echo "File not found"

echo -e "\n=== Checking efficientps.md ==="
fd "efficientps.md" --exec cat {} || echo "File not found"

echo -e "\n=== Checking issue #368 ==="
gh issue view 368 2>/dev/null || echo "Issue not accessible"

Length of output: 17090

Comment on lines +57 to +59
## Follow up Question

Why the last group used bounding boxes and not the segmentation model is it to slow or maybe not reliable?
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve grammar and clarity in the follow-up question.

The question needs better formatting and grammar.

Apply this diff:

-## Follow up Question
+## Follow-up Question
 
-Why the last group used bounding boxes and not the segmentation model is it to slow or maybe not reliable?
+Why did the previous group use bounding boxes instead of the segmentation model? Was it too slow or perhaps not reliable enough?
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Follow up Question
Why the last group used bounding boxes and not the segmentation model is it to slow or maybe not reliable?
## Follow-up Question
Why did the previous group use bounding boxes instead of the segmentation model? Was it too slow or perhaps not reliable enough?
🧰 Tools
🪛 LanguageTool

[grammar] ~57-~57: ‘Follow up’ seems to be a compound adjective before a noun. Use a hyphen: “Follow-up”.
Context: ...era,-The%20semantic%20segmentation> ## Follow up Question Why the last group used bound...

(CA_FOLLOW_UP)


[uncategorized] ~59-~59: Possible missing comma found.
Context: ...bounding boxes and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~59-~59: “to” seems less likely than “too” (as well, very, ‘too much’).
Context: ...es and not the segmentation model is it to slow or maybe not reliable?

(AI_HYDRA_LEO_CP_TO_TOO)

Copy link
Collaborator

@Lukasnol Lukasnol left a comment

Choose a reason for hiding this comment

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

looks good to me

@RoyaLxPole RoyaLxPole merged commit 9ed826b into main Nov 4, 2024
4 checks passed
@Lukasnol Lukasnol deleted the 368-visionnode-and-segmentation branch November 4, 2024 10:45
@coderabbitai coderabbitai bot mentioned this pull request Nov 4, 2024
7 tasks
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.

VisionNode and Segmentation
2 participants