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

feat: add parameter description #171

Conversation

takayuki5168
Copy link
Contributor

@takayuki5168 takayuki5168 commented Jul 26, 2022

Signed-off-by: Takayuki Murooka takayuki5168@gmail.com

Related links

autowarefoundation/autoware_launch#66

Description

We will add a config/ directory in the autoware_launch package soon.
In this PR, I added the document about what kind of parameters we have and how to manage them.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The Reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
@takayuki5168 takayuki5168 marked this pull request as draft July 26, 2022 04:14
@takayuki5168 takayuki5168 added the type:documentation Creating or refining documentation. label Jul 26, 2022
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
@takayuki5168 takayuki5168 changed the title feat: add parameter description [WIP] feat: add parameter description Jul 26, 2022
@takayuki5168
Copy link
Contributor Author

@kenji-miyake
This is a WIP PR.
Could you have a brief look at this PR? Does this PR look good so far?

@kenji-miyake
Copy link
Contributor

@takayuki5168 Could you write a bit more concrete description? 🙇

Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

Thank you for adding this document!
As a developer, it generally looks good to me.
However, it would be better if you can write this document from the user's point of view.
So, let's discuss and improve this document together!

Comment on lines 5 to 9
- `p1`: reference parameter for the node package
- e.g., [the parameter for the `behavior_path_planner` package](https://github.com/autowarefoundation/autoware.universe/tree/main/planning/behavior_path_planner/config)
- `p2`: reference parameter for the module launch
- e.g., [the parameter for the `tier4_planning_launch` package](https://github.com/autowarefoundation/autoware.universe/tree/main/launch/tier4_planning_launch/config/)
- `p3`: [reference parameter for the `autoware_launch` package](https://github.com/autowarefoundation/autoware_launch/tree/main/autoware_launch/config)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about converting this list to a table?
Also, I feel it's better to use more descriptive names for p1, p2, and p3.
For example, Node Parameter, Component Parameter, and Launch Parameter? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is it possible to add a figure to visually explain these types of parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your good idea. I've fixed what I agreed.
The explanation is not short words but sentences, so I feel it better to use itemization rather than the table.

Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
@takayuki5168 takayuki5168 changed the title [WIP] feat: add parameter description feat: add parameter description Jul 26, 2022
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
- e.g., [the parameter for the `behavior_path_planner` package](https://github.com/autowarefoundation/autoware.universe/tree/main/planning/behavior_path_planner/config)
- `Component Parameter`: the reference parameter for the component launch
- e.g., [the parameter for the `tier4_planning_launch` package](https://github.com/autowarefoundation/autoware.universe/tree/main/launch/tier4_planning_launch/config/)
- `Launch Parameter`: [the reference parameter for the `autoware_launch` package](https://github.com/autowarefoundation/autoware_launch/tree/main/autoware_launch/config)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about replacing the term Launch Paramteter with something like

  • Autoware Parameter
  • System Parameter
  • Autoware_launch Parameter

(Launch Parameter sounds a bit ambiguous to me compared to Node or Component)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a system component, so System Parameter is confusing.

Autoware Parameter may be better?
@kenji-miyake What do you think?

Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
@takayuki5168 takayuki5168 marked this pull request as ready for review September 27, 2022 02:46
@takayuki5168
Copy link
Contributor Author

@kenji-miyake Can I merge this PR if you get your approval even though the parameter management described in this doc has not been used in autoware.

I wrote like this to notice that this is a proposal document for now.
image

@kenji-miyake
Copy link
Contributor

@takayuki5168 Hmm, I think in that case we should discuss and adopt it. What is the blocker for using the parameter management method?
Anyway, I'll take a look at this PR this week in more detail.

kminoda and others added 2 commits September 27, 2022 16:58
Signed-off-by: kminoda <koji.m.minoda@gmail.com>
@kenji-miyake
Copy link
Contributor

kenji-miyake commented Oct 19, 2022

(I'm sorry for my late reply.)
I've talked to @takayuki5168. We'll organize the management system of config files first, and merge this document at the same time.

The development policy is already agreed in autowarefoundation/autoware_launch#66.

@stale
Copy link

stale bot commented Dec 18, 2022

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:stale Inactive or outdated issues. (auto-assigned) type:documentation Creating or refining documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants