Refactor mapmatching configuration to use a struct #2485

genadz · 2020-07-22T09:21:29Z

Add configuration struct for mapmatching. It can be initialized with default values or with boost::property_tree (if some parameters are missing -> use default values, don't throw any exception).

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Generally use squash merge to rebase and clean comments before merging
Update the changelog

genadz · 2020-07-22T10:51:03Z

src/meili/config.cc

+}
+
+void Config::CandidateSearch::Read(const boost::property_tree::ptree& params) {
+  ReadParamOptional(search_radius, params, "default.search_radius");


I'm not sure about this: should we treat parameters from json as optional or required ?

I think this it should be optional, with a sensible default configuration. My understanding though is that this is different from the current behavior, but should be fine because we are relaxing (vs strengthening) an API constraint (i.e., what was previously optional is now required).

mookerji · 2020-07-25T00:47:45Z

test/mapmatch_config.cc

+
+namespace {
+
+boost::property_tree::ptree json_to_pt(const std::string& json) {


ACK! We have 12 places where this function is copied. We should add it to a header somewhere and not have 13 places.

See also: #2481 (comment)

/cc @purew

good catch! will fix

mookerji · 2020-07-25T00:49:59Z

valhalla/meili/config.h

+    void Read(const boost::property_tree::ptree& params);
+  };
+
+  struct Emission {


This struct specifically characterizes a likelihood function, so I'd call it EmissionLikelihood. Same for TransitionLikelihood.

It should probably be TransitionCost, etc. since that's existing terminology used in meili.

mookerji · 2020-07-25T00:51:29Z

valhalla/meili/config.h

+
+  struct CandidateSearch {
+    // default search radius (meters) for measurements; it's used to determine maximum search radius
+    float search_radius = 50.f;


For these variables that have SI units, we should add a units suffix, like search_radius_meters.

mookerji · 2020-07-25T01:59:45Z

src/meili/config.cc

+#include "meili/config.h"
+#include "midgard/logging.h"
+
+#define CHECK_POSITIVE_PARAM(value, name)                                                            \


Macro is a great idea, but I think we can generalize this. My thoughts:

Add a macro.h header ... somewhere. I'm not sure what folder to put this in. Move this macro definition to that header so it can be used elsewhere.

Generalize CHECK_POSITIVE_PARAM to a more general conditional check, like a CHECK_THROWS(<condition>, <message>) macro. You could probably define something like:

#define CHECK_THROWS(condition, exc_info) \ do { \ if (!(condition)) { \ throw std::invalid_argument(exc_info); \ } \ } while (0)

I probably butchered the syntax here, but you get the idea.

done. the main idea here was to avoid copying an exception message

mookerji · 2020-07-25T02:51:31Z

valhalla/meili/config.h

+
+  // define options that can be dynamically reassigned with user request
+  struct Customizable {
+    bool turn_penalty_factor = true;


Would it make more sense for each struct here to have a is_customizable=true|false flag?

I thought about it. We can't have one flag for the whole struct because the struct may have customizable and non-customizable fields the same time. I think, the best solution here to remove flags from Customizable struct and put to others that have customizable params (looks like below you suggested the same).

mookerji · 2020-07-25T03:01:23Z

src/meili/config.cc

+  interpolation_distance = false;
+}
+
+void Config::Customizable::Read(const boost::property_tree::ptree& params) {


See comment below about possibly moving the flag into each struct. This property of each configuration should be colocated with each value its describing.

mookerji · 2020-07-25T03:03:53Z

src/meili/map_matcher.cc

@@ -492,21 +492,21 @@ struct path_t {
 namespace valhalla {
 namespace meili {

-MapMatcher::MapMatcher(const boost::property_tree::ptree& config,
+MapMatcher::MapMatcher(const Config& config,


I don't think the API compatibility should be an issue, I've also cc'd some downstream users of the mapmatching library in case they have opinions.

mookerji · 2020-07-25T03:08:38Z

src/thor/worker.cc

@@ -293,8 +293,8 @@ void thor_worker_t::parse_measurements(const Api& request) {

  // we require locations
  try {
-    auto default_accuracy = matcher->config().get<float>("gps_accuracy");
-    auto default_radius = matcher->config().get<float>("search_radius");
+    auto default_accuracy = matcher->config().emission.gps_accuracy;


Let's get rid of these auto's when we see them. I think in this case, it makes the code to understand.

changed for some cases considering readability.

mookerji

This is looking really good! Made some suggestions and eager to hear what you think.

SiarheiFedartsou

Significantly improves readability. LGTM.

test/utils.h

valhalla/macro.h

mookerji · 2020-08-13T04:41:26Z

This is looking really good, @genadz ! I'm out, but hopefully @kevinkreiser can help you get this merged.

* Read config parameters from boost::property_tree::ptree * Add unittest for mapmatching config * Move some common functions from test files to shared header

all questions were addressed

genadz force-pushed the mm-static-config branch 2 times, most recently from bb29886 to 7f1335f Compare July 22, 2020 09:52

genadz commented Jul 22, 2020

View reviewed changes

genadz requested review from yuzheyan and mookerji July 24, 2020 14:46

mookerji requested a review from kevinkreiser July 25, 2020 00:00

mookerji assigned genadz Jul 25, 2020

mookerji reviewed Jul 25, 2020

View reviewed changes

mookerji requested review from SiarheiFedartsou and mskurydin July 25, 2020 03:01

mookerji reviewed Jul 25, 2020

View reviewed changes

mookerji previously requested changes Jul 25, 2020

View reviewed changes

SiarheiFedartsou previously approved these changes Jul 28, 2020

View reviewed changes

genadz dismissed SiarheiFedartsou’s stale review via 251129d August 12, 2020 11:30

genadz force-pushed the mm-static-config branch 4 times, most recently from 2a865fc to 6a2f44b Compare August 12, 2020 13:28

genadz requested a review from mookerji August 12, 2020 13:28

kevinkreiser reviewed Aug 12, 2020

View reviewed changes

test/utils.h Outdated Show resolved Hide resolved

kevinkreiser reviewed Aug 12, 2020

View reviewed changes

valhalla/macro.h Show resolved Hide resolved

genadz requested a review from kevinkreiser August 12, 2020 16:24

genadz force-pushed the mm-static-config branch from 6a2f44b to ee10af8 Compare August 20, 2020 08:46

Add configuration struct for mapmatching

415fa26

* Read config parameters from boost::property_tree::ptree * Add unittest for mapmatching config * Move some common functions from test files to shared header

genadz force-pushed the mm-static-config branch from ee10af8 to 415fa26 Compare August 20, 2020 09:24

kevinkreiser approved these changes Aug 21, 2020

View reviewed changes

genadz requested review from SiarheiFedartsou and removed request for yuzheyan August 21, 2020 15:01

Merge branch 'master' into mm-static-config

947f828

kevinkreiser merged commit 81f7f2b into valhalla:master Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor mapmatching configuration to use a struct #2485

Refactor mapmatching configuration to use a struct #2485

genadz commented Jul 22, 2020 •

edited

Loading

genadz Jul 22, 2020

mookerji Jul 25, 2020

mookerji Jul 25, 2020

genadz Aug 12, 2020

mookerji Jul 25, 2020

mookerji Jul 25, 2020

mookerji Jul 25, 2020

mookerji Jul 25, 2020 •

edited

Loading

genadz Aug 12, 2020

mookerji Jul 25, 2020

genadz Aug 12, 2020

mookerji Jul 25, 2020

mookerji Jul 25, 2020

mookerji Jul 25, 2020

genadz Aug 12, 2020

mookerji left a comment

SiarheiFedartsou left a comment

mookerji commented Aug 13, 2020


		namespace {

		boost::property_tree::ptree json_to_pt(const std::string& json) {

Refactor mapmatching configuration to use a struct #2485

Refactor mapmatching configuration to use a struct #2485

Conversation

genadz commented Jul 22, 2020 • edited Loading

Tasklist

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

mookerji Jul 25, 2020 • edited Loading

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

mookerji left a comment

Choose a reason for hiding this comment

SiarheiFedartsou left a comment

Choose a reason for hiding this comment

mookerji commented Aug 13, 2020

genadz commented Jul 22, 2020 •

edited

Loading

mookerji Jul 25, 2020 •

edited

Loading