-
Notifications
You must be signed in to change notification settings - Fork 809
docs(autoware_lidar_frnet): add UML diagram for processing flow #11660
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
base: main
Are you sure you want to change the base?
docs(autoware_lidar_frnet): add UML diagram for processing flow #11660
Conversation
…e_lidar_frnet package Signed-off-by: Kyoichi Sugahara <[email protected]>
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11660 +/- ##
==========================================
- Coverage 17.79% 17.78% -0.02%
==========================================
Files 1747 1748 +1
Lines 122053 122099 +46
Branches 42794 42813 +19
==========================================
- Hits 21723 21714 -9
- Misses 82134 82214 +80
+ Partials 18196 18171 -25
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
amadeuszsz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyoichi-sugahara
Thank you for your PR!
We definitely need such a inner-algorithm description. However, since proposed diagram is not C++ UML architecture design (not enough details), in some places it seems to me too detailed which increases maintenance cost.
For example, you could just use a single block "Copy outputs from device to host" instead of pointing each tensor separately.
On the other hand, diagram is not detailed enough, or let's say - too ambiguous. The greatness of such a diagrams is that most of users can take a look and understand the whole pipeline. Let me start from beginning of the diagram:
- Input validation does not show what happens if input is invalid.
- I don't what "against profile" means. What is the profile? (I know, but any user even with technical background will get confused). What happens if input is invalid? Wouldn't be better to write it as "Check the input point cloud’s point count against the min/max thresholds".
- What yaw and pitch angles refer to? You could write that it is about sensor's specification, but having in mind previously described detail level, "Project 3D points to 2D frustum" is enough.
- And so on.
Please consider to update this diagram.
|
@amadeuszsz Can I try updating diagram based on your feedback? Or, if you don't think this diagram is strictly necessary, I'm fine with closing this PR 😅 because the main purpose of this PR was to clarify my understanding, and I'm already satisfied. |
Description
I have added a processing flow diagram to visualize the logic flow. This addition is intended to facilitate future discussions regarding code improvements, such as multi-threading and batch processing optimization.
Note:
The diagram is designed for a high-level understanding and does not aim to be strictly comprehensive or cover every single processing step; therefore, some details may be omitted.
I am open to adjusting the granularity of the processes described in the flow. If there are specific policies or preferences regarding how detailed these steps should be, please provide feedback in the comments.
Related links
Parent Issue:
How was this PR tested?
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.