Adds max_linear_tolerance parameter: linear tolerance as a range #182

francocipollone · 2021-10-14T14:01:28Z

Part of #130

Summary

This PR enables setting a range of linear tolerances to be used.

A new parameter is accepted to be used when calling the load(std::map<std::string, std::string> params) method: "max_linear_tolerance"
- When max_linear_tolerance is passed, the builder will create a range of valid linear tolerances: [linear_tolerance, max_linear_tolerance]. Afterwards, the builder will try building the RoadGeometry scaling from the minimum value of linear tolerance to the maximum, doing increments of 10% of the previous linear tolerance.
The ToleranceSelectionPolicy was deprecated, as this is implicitly enabled when the optional parameter max_linear_tolerance is passed.
The old logic of automatic tolerance selection also increased the angular_tolerance in each iteration. In this PR that behavior is changed the angular_tolerance remains the same. Given that in general, the angular_tolerance isn't an issue when loading maps. If we still want to increase the angular_tolerance in each iteration I think that accepting a max_angular_tolerance parameter would be the way to go.

Comments for the reviewers

Reviewing the PR commit by commit is recommended.

liangfok · 2021-10-14T14:22:37Z

maliput_malidrive/include/maliput_malidrive/loader/loader.h

+///                            When this parameters is passed, the linear tolerance the builder will use
+///                            is defined within the range [linear_tolerance, max_linear_tolerance].
+///                            The builder is expected to iteratively try higher linear tolerances as building process
+///                            fails.


Instead of "as building process fails", let's use "until it either finds a value that works, or reaches this maximum value, at which point it will abort with a failure."

Thanks for the suggestion. Done.

liangfok · 2021-10-14T14:23:31Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_builder.cc

+    MALIDRIVE_VALIDATE(
+        rg_config_.tolerances.max_linear_tolerance.value() >= rg_config_.tolerances.linear_tolerance,
+        maliput::common::assertion_error,
+        std::string("max_linear_tolerance should be greater or equal than linear_tolerance.\n linear_tolerance: ") +


"greater or equal than" should be "greater than or equal to".

liangfok · 2021-10-14T14:24:22Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_builder.cc

+        rg_config_.tolerances.max_linear_tolerance.value() >= rg_config_.tolerances.linear_tolerance,
+        maliput::common::assertion_error,
+        std::string("max_linear_tolerance should be greater or equal than linear_tolerance.\n linear_tolerance: ") +
+            std::to_string(rg_config_.tolerances.linear_tolerance) + std::string("\nmax_linear_tolerance: ") +


"\nmax_linear_tolerance

For consistency, should add a space after "\n".

liangfok · 2021-10-14T14:25:43Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_builder.cc

-  std::array<double, constants::kMaxToleranceSelectionRounds + 1> angular_tolerances{};
-  std::array<double, constants::kMaxToleranceSelectionRounds + 1> scale_lengths{};
-
+  // As a max linear tolerance is provided, the tolerance range logic is enabled.


"As a" should be "Because a".

liangfok · 2021-10-14T14:27:44Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_configuration.h

@@ -130,6 +109,31 @@ struct RoadGeometryConfiguration {
  /// @param road_geometry_configuration A string-string map containing the configuration for the builder.
  static RoadGeometryConfiguration FromMap(const std::map<std::string, std::string>& road_geometry_configuration);

+  /// Holds linear and angular tolerance to be used by the builder.
+  /// A range could be selected for linear tolerance, allowing the builder to try
+  /// different values of linear tolerance within that range when the builder process fails.


values of linear tolerance

should be:

values of linear tolerances

Let's replace "when the builder process fails" with "searching for a value that works".

liangfok · 2021-10-14T14:28:47Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_configuration.h

+  /// A range could be selected for linear tolerance, allowing the builder to try
+  /// different values of linear tolerance within that range when the builder process fails.
+  struct BuildTolerance {
+    /// Set linear and angular tolerance.


"Set" should be "Sets" per Google style guide.

(Ditto below.)

liangfok · 2021-10-14T14:33:02Z

maliput_malidrive/include/maliput_malidrive/constants.h

@@ -10,6 +10,9 @@ namespace constants {
 static constexpr double kLinearTolerance{5e-2};   // [m]
 static constexpr double kAngularTolerance{1e-2};  // [rad]
 static constexpr double kScaleLength{1.};         // [m]
+/// Step used to increase the tolerance by the Builder.
+static constexpr double kIncreasingToleranceStep{1.1};


Is this a multiplier? The word "Step" without a qualifier makes me think it is an absolute step size (meter units). Maybe we can use "Multiplier"?

static constexpr double kToleranceStepMultiplier{1.1};

I took away "Increasing" since that can be inferred based on multiplier > 1.

Fully agree. Done.

liangfok · 2021-10-14T14:41:29Z

maliput_malidrive/include/maliput_malidrive/loader/loader.h

@@ -25,6 +25,12 @@ namespace loader {
 ///   - Default: ""
 /// - @b linear_tolerance : RoadGeometry's linear tolerance.
 ///   - Default: @e "5e-2" (#malidrive::constants::kLinearTolerance)


This got me thinking: given max_linear_tolerance, is there ever a reason to specify a minimum linear tolerance? If the builder is able to find a linear tolerance that is smaller than the specified minimum, is there a reason not to use it?

If smaller linear tolerances are always better, could we allow users to only specify the max linear tolerance, and have the loader start searching from a very small value that is epsilon above zero?

My concern about starting with a minimum epsilon value (an epsilon to be defined) is the time that it could take in achieving the right tolerance. However, if the user is willing to accept that time then it could be useful.

I wouldn't remove the linear_tolerance variable given that it would be a hint for the builder when it is known that a certain map won't support tolerances less than that linear_tolerance.

So at the end the user could do:

set only linear_tolerance: Builder will only try with this linear tolerance

set only max_linear_tolerance: Builder will start the iteration process from a small value(epsilon TBD)

set both linear_tolerance and max_linear_tolerance: Builder will iterate from the minimum to the maximum value of the range delimited by linear_tolerance and max_linear_tolerance

WDYT?

Sound good. Let's just document the fact that it might take a lot of time if the user only specifies a max tolerance, but the benefit is potentially lower tolerance.

While implementing this new behavior I found that there was one verification that was being done at a parser level ~~that wasn't taken into account during this tolerance selection mechanism~~: #183 (Edited: Mentioned in the issue: Actually it was taken into account however it was causing unnecessary extra time when iterating the tolerances)

So I pushed #185 and after this is merged and I will push the here-discussed implementation about the linear_tolerance and max_linear_tolerance flags.

So at the end the user could do:
set only linear_tolerance: Builder will only try with this linear tolerance
set only max_linear_tolerance: Builder will start the iteration process from a small value(epsilon TBD)
set both linear_tolerance and max_linear_tolerance: Builder will iterate from the minimum to the maximum value of the range delimited by linear_tolerance and max_linear_tolerance

I've addressed this at:

11a0c4c

I also added some tests:

a6c5d4d

liangfok · 2021-10-14T14:58:29Z

maliput_malidrive/test/regression/builder/road_geometry_builder_test.cc

-        RoadGeometryConfiguration::ToleranceSelectionPolicy::kAutomaticSelection;
+    // By default the RoadGeometryBuilderBaseTest::SetUp() is using GetRoadGeometryConfigurationFor() to get the
+    // RoadGeometryConfigurations which already define a max_linear_tolerance.
+    // However a new max linear tolerance is defined just to the purpose of the test.


"to the purpose" should be "for the purpose".

Thanks. Done.

liangfok · 2021-10-18T18:04:09Z

maliput_malidrive/include/maliput_malidrive/loader/loader.h

+///   - Default: By default it isn't set. When `max_linear_tolerance` isn't also set then @e "5e-2"
+///   (#malidrive::constants::kLinearTolerance) is used.
+/// - @b max_linear_tolerance : A maximum allowed linear tolerance.
+///                            When this parameters is passed, the linear tolerance the builder will use


"this parameters" should be "this parameter".

Thanks. Done.

liangfok · 2021-10-18T18:09:31Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_builder.h

  /// `road_geometry_configuration.scale_length` are negative.
-  /// `manager` or `factory` are nullptr.
+  /// @throws maliput::common::assertion_error When `road_geometry_configuration.tolerances.max_linear_tolerance` is
+  /// less than `road_geometry_configuration.tolerances.linear_tolerance`,


The comma should be a period.

liangfok · 2021-10-18T18:09:33Z

maliput_malidrive/src/maliput_malidrive/builder/road_geometry_builder.h

-  /// `manager` or `factory` are nullptr.
+  /// @throws maliput::common::assertion_error When `road_geometry_configuration.tolerances.max_linear_tolerance` is
+  /// less than `road_geometry_configuration.tolerances.linear_tolerance`,
+  /// @throws maliput::common::assertion_error When `manager` are nullptr.


"are" should be "is".

liangfok

LGTM

francocipollone added 4 commits October 14, 2021 10:34

Adds max_linear_tolerance parameter.

c14ee3b

Modifies RoadGeometryBuilder to enable the tolerance range.

b76f682

Updates the GetRoadGeometryConfigurationFor() method.

e656a93

Updates tests.

1d81daa

francocipollone requested a review from liangfok October 14, 2021 14:13

francocipollone mentioned this pull request Oct 14, 2021

Includes malidrive's new parameter: max_linear_tolerance maliput/maliput_integration#99

Merged

liangfok requested changes Oct 14, 2021

View reviewed changes

francocipollone added 3 commits October 14, 2021 16:19

Addresses Liang's comments.

6e1961e

Enable tolerance selection logic when only max tol is passed.

11a0c4c

Adds tests.

a6c5d4d

francocipollone requested a review from liangfok October 18, 2021 17:33

liangfok requested changes Oct 18, 2021

View reviewed changes

Liang's comments are addressed.

9d1d13c

francocipollone requested a review from liangfok October 18, 2021 18:15

liangfok approved these changes Oct 18, 2021

View reviewed changes

francocipollone merged commit 7d07074 into main Oct 18, 2021

francocipollone deleted the francocipollone/add_tolerance_range branch October 18, 2021 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds max_linear_tolerance parameter: linear tolerance as a range #182

Adds max_linear_tolerance parameter: linear tolerance as a range #182

francocipollone commented Oct 14, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 14, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021 •

edited

francocipollone Oct 18, 2021

liangfok Oct 14, 2021

francocipollone Oct 18, 2021

liangfok Oct 18, 2021

francocipollone Oct 18, 2021

liangfok Oct 18, 2021

francocipollone Oct 18, 2021

liangfok Oct 18, 2021

francocipollone Oct 18, 2021

liangfok left a comment

Adds max_linear_tolerance parameter: linear tolerance as a range #182

Adds max_linear_tolerance parameter: linear tolerance as a range #182

Conversation

francocipollone commented Oct 14, 2021

Summary

Comments for the reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francocipollone Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangfok left a comment

Choose a reason for hiding this comment

francocipollone Oct 18, 2021 •

edited