Conversation
… consistency on all code parts then
… consistency on all code parts then
|
@niosus, could you have a look at C++ part? Thank you :) |
niosus
left a comment
niosus
left a comment
There was a problem hiding this comment.
Overall, the logic is easy to follow and the code is well written and styled. So I am approving the PR.
That being said, there was a couple of places where the code can be slightly improved to follow best practices and reduce the potential for future errors creeping up. Please see the comments in the code for those.
| namespace loc = localization; | ||
|
|
||
| std::vector<std::unique_ptr<loc::features::iFeature>> | ||
| loadFeatures(const std::string &path2folder) { |
There was a problem hiding this comment.
It is a good practice to put functions in a cpp file into an unnamed namespace to enforce internal linkage of these functions, i.e., to guarantee that they are only visible from this file. So I would put the loadFeatures function in one:
namespace {
std::vector<std::unique_ptr<loc::features::iFeature>>
loadFeatures(const std::string &path2folder) {...}
} // namespace| LOG(INFO) << "Loading the features to hash with LSH."; | ||
| std::vector<std::string> featureNames = | ||
| loc::database::listProtoDir(path2folder, ".Feature"); | ||
| std::vector<std::unique_ptr<loc::features::iFeature>> features; |
There was a problem hiding this comment.
nit: you might want to reserve the memory for the features if you have a lot of those:
features.reserve(featureNames.size());| LOG(INFO) << "===== Online place recognition with LSH ====\n"; | ||
|
|
||
| if (argc < 2) { | ||
| printf("[ERROR] Not enough input parameters.\n"); |
There was a problem hiding this comment.
nit: why not use LOG(FATAL) << "message" instead? It also terminates the program with an error code.
| if (argc < 2) { | ||
| printf("[ERROR] Not enough input parameters.\n"); | ||
| printf("Proper usage: ./cost_matrix_based_matching_lsh config_file.yaml\n"); | ||
| exit(0); |
There was a problem hiding this comment.
nit: Follow up on above, if a program terminates with an error it probably should not return code 0 as it will be interpreted as "program terminated correctly".
|
|
||
| std::string config_file = argv[1]; | ||
| ConfigParser parser; | ||
| parser.parseYaml(config_file); |
There was a problem hiding this comment.
nit: a small design suggestions that might make sense here. Why now pass the config_file into the constructor of the ConfigParser directly?
| /*tableNum=*/1, | ||
| /*keySize=*/12, | ||
| /*multiProbeLevel=*/2); |
There was a problem hiding this comment.
nit: it is a good practice to put such magic numbers as constexpr int kTableNum etc into the unnamed namespace at the top of the file. This way they are easy to find upon glancing into the file.
| /*multiProbeLevel=*/2); | ||
| relocalizer->train(loadFeatures(parser.path2ref)); | ||
|
|
||
| auto successorManager = |
| /*type=*/loc::features::FeatureType::Cnn_Feature, | ||
| /*bufferSize=*/parser.bufferSize); | ||
|
|
||
| auto relocalizer = std::make_unique<loc::relocalizers::LshCvHashing>( |
| std::string path2refImg = ""; | ||
| std::string imgExt = ""; | ||
| std::string costMatrix = ""; | ||
| std::string costsOutputName = "costs.MatchingCosts.pb"; |
There was a problem hiding this comment.
Generally speaking, using default values on which your logic depends is frowned upon. The reason is that if your logic changes, these default values are easy to forget. I would suggest to set the default value to empty and just pass the appropriate values into a constructor that takes just the values you need. This way, when reading the code it should be easy to see which objects are created with which values.
| } | ||
|
|
||
| // Is used to store matching costs with associated query and reference id. | ||
| // Does not require all elements of the cost matrix to be present |
There was a problem hiding this comment.
These comments usually go out of date quite quickly and then become confusing to read. Maybe jut add the check whenever you read the message and remove this comment?
2 participants
This PR adds new message type to
localization_protos. This type is called "MatchingCosts" and is used to store individual matching costs for a pair of images with [query_id, ref_id]. The difference to "CostMatrix" is that "MatchingCosts" are not required to store all values of the cost matrix of sizerow x colbut can store just individual values. This is necessary to be able to visualize the "online" operation of the sequence matching algorithm.Additionally, this PR adds functionality to read and write "MatchingCosts" from C++, Python and viewer interfaces.