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

refactor(perception_utils): add test in object_classification #2083

Merged

Conversation

YoshiRi
Copy link
Contributor

@YoshiRi YoshiRi commented Oct 17, 2022

Description

add test function for object_classification.hpp in common/perception_util.

Here is test coverage.
image

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.

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
@takayuki5168
Copy link
Contributor

takayuki5168 commented Oct 17, 2022

@YoshiRi Thanks for your PR.
The title of the PR should be refactor(package_name): , and the package name is perception_utils.

@YoshiRi YoshiRi changed the title refactor(perception_util)/add test in object_classification refactor(perception_utils)/add test in object_classification Oct 17, 2022
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
@YoshiRi YoshiRi force-pushed the refactor/add_test_perception_util branch from 6806a6b to 8e74090 Compare October 17, 2022 09:15
classification.push_back(createObjectClassification(ObjectClassification::MOTORCYCLE, 0.8));
classification.push_back(createObjectClassification(ObjectClassification::BICYCLE, 0.8));
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 adding a test to see what will happen if the object classification includes car-like vehicle and not car-like vehicle with the same probability.
This will be the edge case for isCarLikeVehicle function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I'll do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This edge case depends following function getHighestProbLabel, which extract highest-scored label from classification.

  • This function do not considering multiple highest probability
  • It returns an earlier label with maximum probability. (It depends labels order!)
  • For example, if an objects has CAR and MOTORCYCLE label with the same probability,
    • isCarLikeVehicle return true when CAR label is added first
    • else it returns false
inline std::uint8_t getHighestProbLabel(
  const std::vector<ObjectClassification> & object_classifications)
{
  std::uint8_t label = ObjectClassification::UNKNOWN;
  float highest_prob = 0.0;
  // TODO(Satoshi Tanaka): It might be simple if you use STL or range-v3.
  for (const auto & object_classification : object_classifications) {
    if (highest_prob < object_classification.probability) {
      highest_prob = object_classification.probability;
      label = object_classification.label;
    }
  }
  return label;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Even if the current implementation is not good, in order to show how it works with the edge case in the current implementation, adding this edge-case test seems useful.
What do you think?

Copy link
Contributor Author

@YoshiRi YoshiRi Oct 17, 2022

Choose a reason for hiding this comment

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

Frankly, I think it is a rare situation that the label probabilities become the same since it is float value.
But adding edge case seems to be not harmful and it is easy to implement, I added some edge cases to this PR.

Thanks for your help.

@@ -61,3 +61,114 @@ TEST(object_classification, test_getHighestProbLabel)
EXPECT_EQ(label, ObjectClassification::CAR);
Copy link
Contributor

Choose a reason for hiding this comment

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

image

pre-commit failed. Don't you know how to fix it on a github browser or locally with a command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be due to unnecessary line break in the code.
I fixed it in local and push later.

Copy link
Contributor

@takayuki5168 takayuki5168 Oct 17, 2022

Choose a reason for hiding this comment

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

I meant there is an auto-fix command in both github browser and local command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot pre-commit run -a command before git push.
(But I still had to fix some error manually)

Copy link
Contributor

@takayuki5168 takayuki5168 Oct 17, 2022

Choose a reason for hiding this comment

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

Got it.

pre-commit install which enables running pre-commit every time we run git commit is also useful.

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
@YoshiRi YoshiRi force-pushed the refactor/add_test_perception_util branch from aa8d873 to 7bdb3a3 Compare October 17, 2022 09:57
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
@YoshiRi YoshiRi force-pushed the refactor/add_test_perception_util branch from e372c33 to c1182c2 Compare October 17, 2022 10:13
@YoshiRi YoshiRi changed the title refactor(perception_utils)/add test in object_classification refactor(perception_utils): add test in object_classification Oct 17, 2022
Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Base: 10.75% // Head: 10.37% // Decreases project coverage by -0.38% ⚠️

Coverage data is based on head (f4689cb) compared to base (ea27b08).
Patch coverage: 20.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2083      +/-   ##
==========================================
- Coverage   10.75%   10.37%   -0.39%     
==========================================
  Files        1184     1171      -13     
  Lines       84694    84223     -471     
  Branches    19833    19652     -181     
==========================================
- Hits         9110     8738     -372     
+ Misses      65958    65916      -42     
+ Partials     9626     9569      -57     
Flag Coverage Δ *Carryforward flag
differential 5.71% <20.00%> (?)
total 10.39% <ø> (-0.34%) ⬇️ Carriedforward from 62e8f55

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

Impacted Files Coverage Δ
...tion_utils/test/src/test_object_classification.cpp 20.00% <20.00%> (+3.87%) ⬆️
...lude/freespace_planning_algorithms/reeds_shepp.hpp 50.00% <0.00%> (-50.00%) ⬇️
.../freespace_planning_algorithms/src/reeds_shepp.cpp 55.51% <0.00%> (-38.97%) ⬇️
...eespace_planning_algorithms/abstract_algorithm.hpp 44.44% <0.00%> (-25.56%) ⬇️
...lude/behavior_path_planner/turn_signal_decider.hpp 25.00% <0.00%> (-25.00%) ⬇️
...ehicle/raw_vehicle_cmd_converter/src/accel_map.cpp 19.23% <0.00%> (-21.51%) ⬇️
...hicle/raw_vehicle_cmd_converter/src/csv_loader.cpp 53.84% <0.00%> (-16.37%) ⬇️
...ehicle/raw_vehicle_cmd_converter/src/brake_map.cpp 17.85% <0.00%> (-15.48%) ⬇️
...ace_planning_algorithms/src/abstract_algorithm.cpp 78.16% <0.00%> (-5.54%) ⬇️
...utoware_utils/src/geometry/boost_polygon_utils.cpp 77.77% <0.00%> (-2.47%) ⬇️
... and 98 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@takayuki5168
Copy link
Contributor

@YoshiRi Could you resolve the conflict. Then this PR is ready to merge, right?

Copy link
Contributor

@takayuki5168 takayuki5168 left a comment

Choose a reason for hiding this comment

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

LGTM

@yukke42
Copy link
Contributor

yukke42 commented Nov 9, 2022

@YoshiRi Have you forgotten to merge?

@YoshiRi YoshiRi merged commit 36b7001 into autowarefoundation:main Nov 9, 2022
HansRobo pushed a commit to HansRobo/autoware.universe that referenced this pull request Dec 16, 2022
…refoundation#2083)

* add test for isVehicle

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* add isCarLikeVehicle test

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* add isLargeVehicle test

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* fix namespace range

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* ci(pre-commit): autofix

* remove unnecessary forloop

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* fix cpplint message

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* added edge case to iscarlikevehicle

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* run precommit

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* add possibe edge cases with comment

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Kotaro Yoshimoto <pythagora.yoshimoto@gmail.com>
YoshiRi added a commit to YoshiRi/autoware.universe that referenced this pull request Jan 11, 2023
…refoundation#2083)

* add test for isVehicle

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* add isCarLikeVehicle test

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* add isLargeVehicle test

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* fix namespace range

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* ci(pre-commit): autofix

* remove unnecessary forloop

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* fix cpplint message

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* added edge case to iscarlikevehicle

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* run precommit

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

* add possibe edge cases with comment

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>

Signed-off-by: Yoshi Ri <yoshi.ri@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
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.

4 participants