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.

Sign up for GitHub

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

Jump to bottom

chore(tensorrt_yolox): rework parameters #6239

Merged
merged 9 commits into from
Feb 26, 2024
Merged

chore(tensorrt_yolox): rework parameters #6239

merged 9 commits into from
Feb 26, 2024

Conversation

tzhong518
Copy link
Contributor

Description

Use config for parameters

Tests performed

Launch tensorrt_yolox and confirm that it loads parameters successfully

Effects on system behavior

Not effect.

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 PR 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.

@tzhong518
chore: use config
51e55aa 
Signed-off-by: tzhong518 <[email protected]>
@github-actions github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jan 30, 2024
@tzhong518 tzhong518 marked this pull request as ready for review January 30, 2024 13:33
kminoda
kminoda previously approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

LGTM

@kminoda kminoda dismissed their stale review February 1, 2024 09:37

Found a bug

@tzhong518
fix: rename
d88251c 
Signed-off-by: tzhong518 <[email protected]>
@tzhong518
Merge branch 'chore/tensorrt_yolox/use_config' of github.com:tzhong51…
958fa82 
…8/autoware.universe into chore/tensorrt_yolox/use_config
@kminoda kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Feb 2, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 2, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 14.64%. Comparing base (b86a00d) to head (ab43171).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #6239   +/-   ##
=======================================
  Coverage   14.64%   14.64%           
=======================================
  Files        1899     1899           
  Lines      130282   130282           
  Branches    38311    38311           
=======================================
  Hits        19083    19083           
  Misses      89784    89784           
  Partials    21415    21415
Flag Coverage Δ *Carryforward flag
differential 0.00% <ø> (?)
total 14.64% <ø> (ø) Carriedforward from b86a00d

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kminoda
Copy link
Contributor

kminoda commented Feb 9, 2024

@manato Hi, would you also check this PR?

@manato
Copy link
Contributor

manato commented Feb 9, 2024

@kminoda Thank you very much for your update! The modification looks good to me roughly.
One quick question: By introducing this modification, the descriptions of each variable look disappear. I guess these descriptions are useful for users who would like to tune the variables. Do you think you can leave them as comments in the yaml? I'd appreciate it if you ignore the proposal in case that it violates some agreements,

@kminoda
Copy link
Contributor

kminoda commented Feb 13, 2024

@manato Thank you for the comment. Agreed.

@tzhong518 Hi, would you also add JSON schema for this PR and include the description of each parameter? I guess that's the current standard for Autoware packages.

@kminoda
Copy link
Contributor

kminoda commented Feb 19, 2024

@tzhong518 Friendly ping

@tzhong518
fix: add json schema
f6cae45 
Signed-off-by: tzhong518 <[email protected]>
@tzhong518
Copy link
Contributor Author

tzhong518 commented Feb 19, 2024 •
edited
Loading

@tzhong518 Friendly ping

JSON schema is added in f6cae45

@kminoda
Copy link
Contributor

kminoda commented Feb 26, 2024

@manato Hi, would you check this again?

manato
manato approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@manato manato left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@kminoda kminoda merged commit 754742e into autowarefoundation:main Feb 26, 2024
22 of 24 checks passed
StepTurtle pushed a commit to StepTurtle/autoware.universe that referenced this pull request Feb 28, 2024
@tzhong518 @pre-commit-ci
@kminoda @StepTurtle
chore(tensorrt_yolox): rework parameters (autowarefoundation#6239)
0c060a2 
* chore: use config

Signed-off-by: tzhong518 <[email protected]>

* style(pre-commit): autofix

* fix: rename

Signed-off-by: tzhong518 <[email protected]>

* fix: add json schema

Signed-off-by: tzhong518 <[email protected]>

* style(pre-commit): autofix

* fix: add comment to param.yaml

Signed-off-by: tzhong518 <[email protected]>

---------

Signed-off-by: tzhong518 <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: kminoda <[email protected]>
HansRobo pushed a commit that referenced this pull request Mar 12, 2024
@tzhong518 @pre-commit-ci
@kminoda @HansRobo
chore(tensorrt_yolox): rework parameters (#6239)
5e86df6 
* chore: use config

Signed-off-by: tzhong518 <[email protected]>

* style(pre-commit): autofix

* fix: rename

Signed-off-by: tzhong518 <[email protected]>

* fix: add json schema

Signed-off-by: tzhong518 <[email protected]>

* style(pre-commit): autofix

* fix: add comment to param.yaml

Signed-off-by: tzhong518 <[email protected]>

---------

Signed-off-by: tzhong518 <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: kminoda <[email protected]>
Signed-off-by: Kotaro Yoshimoto <[email protected]>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
@tzhong518 @pre-commit-ci
@kminoda @karishma1911
chore(tensorrt_yolox): rework parameters (autowarefoundation#6239)
2c1ef99 
* chore: use config

Signed-off-by: tzhong518 <[email protected]>

* style(pre-commit): autofix

* fix: rename

Signed-off-by: tzhong518 <[email protected]>

* fix: add json schema

Signed-off-by: tzhong518 <[email protected]>

* style(pre-commit): autofix

* fix: add comment to param.yaml

Signed-off-by: tzhong518 <[email protected]>

---------

Signed-off-by: tzhong518 <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: kminoda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manato manato manato approved these changes

@miursh miursh miursh approved these changes

@kminoda kminoda kminoda approved these changes

@wep21 wep21 Awaiting requested review from wep21

@dan-dnn dan-dnn Awaiting requested review from dan-dnn

Assignees
No one assigned
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tzhong518 @kminoda @manato @miursh