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

Localizer/works without cost matrix #32

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/apps/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
add_subdirectory(cost_matrix_based_matching)
add_subdirectory(cost_matrix_based_matching)
add_subdirectory(feature_based_matching)
15 changes: 15 additions & 0 deletions src/apps/feature_based_matching/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
find_package(OpenCV REQUIRED)

add_executable(feature_based_localizer feature_based_localizer.cpp)
target_link_libraries(feature_based_localizer
cxx_flags
glog::glog
path_element
online_database
online_localizer
successor_manager
config_parser
lsh_cv_hashing
cnn_feature
${OpenCV_LIBS}
)
81 changes: 81 additions & 0 deletions src/apps/feature_based_matching/feature_based_localizer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Created by O.Vysotska in 2023

#include "online_localizer/online_localizer.h"
#include "database/idatabase.h"
#include "database/list_dir.h"
#include "database/online_database.h"
#include "features/cnn_feature.h"
#include "features/ifeature.h"
#include "online_localizer/path_element.h"
#include "relocalizers/lsh_cv_hashing.h"
#include "tools/config_parser/config_parser.h"

#include <glog/logging.h>

#include <iostream>
#include <memory>
#include <string>

namespace loc = localization;

std::vector<std::unique_ptr<loc::features::iFeature>>
loadFeatures(const std::string &path2folder) {
Copy link

Choose a reason for hiding this comment

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

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;
Copy link

Choose a reason for hiding this comment

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

nit: you might want to reserve the memory for the features if you have a lot of those:

features.reserve(featureNames.size());


for (size_t i = 0; i < featureNames.size(); ++i) {
features.emplace_back(
std::make_unique<loc::features::CnnFeature>(featureNames[i]));
fprintf(stderr, ".");
}
fprintf(stderr, "\n");
LOG(INFO) << "Features were loaded and binarized";
return features;
}

int main(int argc, char *argv[]) {
google::InitGoogleLogging(argv[0]);
FLAGS_logtostderr = 1;
LOG(INFO) << "===== Online place recognition with LSH ====\n";

if (argc < 2) {
printf("[ERROR] Not enough input parameters.\n");
Copy link

Choose a reason for hiding this comment

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

nit: why not use LOG(FATAL) << "message" instead? It also terminates the program with an error code.

printf("Proper usage: ./cost_matrix_based_matching_lsh config_file.yaml\n");
exit(0);
Copy link

Choose a reason for hiding this comment

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

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);
Copy link

Choose a reason for hiding this comment

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

nit: a small design suggestions that might make sense here. Why now pass the config_file into the constructor of the ConfigParser directly?

parser.print();

const auto database = std::make_unique<loc::database::OnlineDatabase>(
/*queryFeaturesDir=*/parser.path2qu,
/*refFeaturesDir=*/parser.path2ref,
/*type=*/loc::features::FeatureType::Cnn_Feature,
/*bufferSize=*/parser.bufferSize);
Comment on lines +55 to +58
Copy link

Choose a reason for hiding this comment

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

nit: these comments indicate that the names in the parser could have been better 😉


auto relocalizer = std::make_unique<loc::relocalizers::LshCvHashing>(
Copy link

Choose a reason for hiding this comment

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

Can it be const?

/*onlineDatabase=*/database.get(),
/*tableNum=*/1,
/*keySize=*/12,
/*multiProbeLevel=*/2);
Comment on lines +62 to +64
Copy link

Choose a reason for hiding this comment

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

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.

relocalizer->train(loadFeatures(parser.path2ref));

auto successorManager =
Copy link

Choose a reason for hiding this comment

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

Can it be const?

std::make_unique<loc::successor_manager::SuccessorManager>(
database.get(), relocalizer.get(), parser.fanOut);
loc::online_localizer::OnlineLocalizer localizer{
successorManager.get(), parser.expansionRate, parser.nonMatchCost};
const loc::online_localizer::Matches imageMatches =
localizer.findMatchesTill(parser.querySize);
loc::online_localizer::storeMatchesAsProto(imageMatches,
parser.matchingResult);
loc::database::storeCostsAsProto(database->getEstimatedCosts(),
parser.costsOutputName);

LOG(INFO) << "Done.";
return 0;
}
28 changes: 28 additions & 0 deletions src/localization/database/online_database.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "database/list_dir.h"
#include "features/feature_buffer.h"
#include "features/ifeature.h"
#include "localization_protos.pb.h"

#include <glog/logging.h>

Expand Down Expand Up @@ -102,4 +103,31 @@ const features::iFeature &OnlineDatabase::getQueryFeature(int quId) {
return addFeatureIfNeeded(*queryBuffer_, quFeaturesNames_, featureType_,
quId);
}

void storeCostsAsProto(
const localization::database::OnlineDatabase::MatchingCosts &costs,
const std::string &protoFilename) {
if (costs.empty()) {
LOG(WARNING) << "Matching costs are empty. Nothing to store.";
return;
}
image_sequence_localizer::MatchingCosts costsProto;
for (const auto &[queryId, refValueMap] : costs) {
for (const auto &[refId, value] : refValueMap) {
image_sequence_localizer::MatchingCosts::Element *element =
costsProto.add_elements();
element->set_query_id(queryId);
element->set_ref_id(refId);
element->set_value(value);
}
}
std::fstream out(protoFilename,
std::ios::out | std::ios::trunc | std::ios::binary);
if (!costsProto.SerializeToOstream(&out)) {
LOG(ERROR) << "Couldn't open the file" << protoFilename;
return;
}
out.close();
LOG(INFO) << "Wrote matching costs to: " << protoFilename;
}
} // namespace localization::database
10 changes: 9 additions & 1 deletion src/localization/database/online_database.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,15 @@
#include <vector>

namespace localization::database {

/**
* @brief Database for loading and matching features. Caches the computed
* matching costs.
*/
class OnlineDatabase : public iDatabase {
public:
using MatchingCosts =
std::unordered_map<int, std::unordered_map<int, double>>;
OnlineDatabase(const std::string &queryFeaturesDir,
const std::string &refFeaturesDir, features::FeatureType type,
int bufferSize, const std::string &costMatrixFile = "");
Expand All @@ -54,6 +57,7 @@ class OnlineDatabase : public iDatabase {
double computeMatchingCost(int quId, int refId);

const features::iFeature &getQueryFeature(int quId);
const MatchingCosts &getEstimatedCosts() const { return costs_; }

protected:
std::vector<std::string> quFeaturesNames_;
Expand All @@ -64,10 +68,14 @@ class OnlineDatabase : public iDatabase {
private:
std::unique_ptr<features::FeatureBuffer> refBuffer_{};
std::unique_ptr<features::FeatureBuffer> queryBuffer_{};
std::unordered_map<int, std::unordered_map<int, double>> costs_;
MatchingCosts costs_;

std::optional<CostMatrix> precomputedCosts_ = {};
};

void storeCostsAsProto(
const localization::database::OnlineDatabase::MatchingCosts &matches,
const std::string &protoFilename);
} // namespace localization::database

#endif // SRC_DATABASE_ONLINE_DATABASE_H_
14 changes: 7 additions & 7 deletions src/localization/online_localizer/online_localizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,21 +92,21 @@ Matches OnlineLocalizer::findMatchesTill(int queryId) {
}

void OnlineLocalizer::writeOutExpanded(const std::string &filename) const {
image_sequence_localizer::Patch patch;
image_sequence_localizer::MatchingCosts matchedCosts;
for (const auto &node : expandedRecently_) {
image_sequence_localizer::Patch::Element *element = patch.add_elements();
element->set_row(node.quId);
element->set_col(node.refId);
element->set_similarity_value(node.idvCost);
image_sequence_localizer::MatchingCosts::Element *element = matchedCosts.add_elements();
element->set_query_id(node.quId);
element->set_ref_id(node.refId);
element->set_value(node.idvCost);
}
std::fstream out(filename,
std::ios::out | std::ios::trunc | std::ios::binary);
if (!patch.SerializeToOstream(&out)) {
if (!matchedCosts.SerializeToOstream(&out)) {
LOG(ERROR) << "Couldn't open the file" << filename;
return;
}
out.close();
LOG(INFO) << "Wrote patch " << filename;
LOG(INFO) << "Wrote matched costs to: " << filename;
}

// frontier picking up routine
Expand Down
10 changes: 5 additions & 5 deletions src/localization/tools/config_parser/config_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ bool ConfigParser::parse(const std::string &iniFile) {
ss >> costMatrix;
continue;
}
if (header == "costOutputName") {
if (header == "costsOutputName") {
ss >> header; // reads "="
ss >> costOutputName;
ss >> costsOutputName;
continue;
}
if (header == "simPlaces") {
Expand Down Expand Up @@ -139,7 +139,7 @@ void ConfigParser::print() const {
printf("== Buffer size: %d\n", bufferSize);

printf("== CostMatrix: %s\n", costMatrix.c_str());
printf("== costOutputName: %s\n", costOutputName.c_str());
printf("== costsOutputName: %s\n", costsOutputName.c_str());
printf("== matchingResult: %s\n", matchingResult.c_str());
printf("== simPlaces: %s\n", simPlaces.c_str());
}
Expand Down Expand Up @@ -187,8 +187,8 @@ bool ConfigParser::parseYaml(const std::string &yamlFile) {
if (config["costMatrix"]) {
costMatrix = config["costMatrix"].as<std::string>();
}
if (config["costOutputName"]) {
costOutputName = config["costOutputName"].as<std::string>();
if (config["costsOutputName"]) {
costsOutputName = config["costsOutputName"].as<std::string>();
}
if (config["simPlaces"]) {
simPlaces = config["simPlaces"].as<std::string>();
Expand Down
44 changes: 22 additions & 22 deletions src/localization/tools/config_parser/config_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,28 +30,28 @@
* @brief Class for storing the configuration parameters.
*/
class ConfigParser {
public:
ConfigParser() {}
bool parse(const std::string &iniFile);
bool parseYaml(const std::string &yamlFile);
void print() const;
public:
ConfigParser() {}
bool parse(const std::string &iniFile);
bool parseYaml(const std::string &yamlFile);
void print() const;

std::string path2qu = "";
std::string path2ref = "";
std::string path2quImg = "";
std::string path2refImg = "";
std::string imgExt = "";
std::string costMatrix = "";
std::string costOutputName = "";
std::string simPlaces = "";
std::string hashTable = "";
std::string matchingResult = "matches.MatchingResult.pb";
std::string path2qu = "";
std::string path2ref = "";
std::string path2quImg = "";
std::string path2refImg = "";
std::string imgExt = "";
std::string costMatrix = "";
std::string costsOutputName = "costs.MatchingCosts.pb";
Copy link

Choose a reason for hiding this comment

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

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.

std::string simPlaces = "";
std::string hashTable = "";
std::string matchingResult = "matches.MatchingResult.pb";

int querySize = -1;
int fanOut = -1;
int bufferSize = -1;
double nonMatchCost = -1.0;
double expansionRate = -1.0;
int querySize = -1;
int fanOut = -1;
int bufferSize = -1;
double nonMatchCost = -1.0;
double expansionRate = -1.0;
};

/*! \var std::string ConfigParser::path2qu
Expand All @@ -73,7 +73,7 @@ class ConfigParser {
/*! \var std::string ConfigParser::costMatrix
\brief stores path to precomputed cost/similarity matrix.
*/
/*! \var std::string ConfigParser::costOutputName
/*! \var std::string ConfigParser::costsOutputName
\brief stores the name of the produced result for the cost_matrix_based
matching.
*/
Expand Down Expand Up @@ -111,4 +111,4 @@ class ConfigParser {
typically be selected from 0.5 - 0.7.
*/

#endif // SRC_TOOLS_CONFIG_PARSER_CONFIG_PARSER_H_
#endif // SRC_TOOLS_CONFIG_PARSER_CONFIG_PARSER_H_
22 changes: 13 additions & 9 deletions src/localization_protos.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,25 @@ syntax = "proto2";

package image_sequence_localizer;

// Assumes that all values of the matrix is known.
// values.size() == rows*cols. No checks for this though.
message CostMatrix {
optional int32 rows = 20;
optional int32 cols = 21;
repeated double values = 1;
}

// Is used to store matching costs with associated query and reference id.
// Does not require all elements of the cost matrix to be present
Copy link

Choose a reason for hiding this comment

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

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?

message MatchingCosts{
message Element {
optional int32 query_id = 1;
optional int32 ref_id = 2;
optional double value = 3;
}
repeated Element elements = 1;
}

message MatchingResult {
message Match {
optional int32 query_id = 1;
Expand All @@ -25,12 +38,3 @@ message Feature {
optional int32 size = 2;
optional string type = 3;
}

message Patch {
message Element {
optional int32 row = 1;
optional int32 col = 2;
optional int32 similarity_value = 3;
}
repeated Element elements = 1;
}
15 changes: 7 additions & 8 deletions src/python/protos_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ def write_feature(filename, proto):


def write_cost_matrix(cost_matrix, cost_matrix_file):

cost_matrix_proto = loc_protos.CostMatrix()
cost_matrix_proto.rows = cost_matrix.shape[0]
cost_matrix_proto.cols = cost_matrix.shape[1]
Expand Down Expand Up @@ -51,14 +50,14 @@ def read_matching_result(filename):


def read_expanded_mask(expanded_patches_dir):
patch_files = list(expanded_patches_dir.glob("*.Patch.pb"))
patch_files.sort()
matching_costs_files = list(expanded_patches_dir.glob("*.MatchingCosts.pb"))
matching_costs_files.sort()

mask = []
for patch_file in patch_files:
f = open(patch_file, "rb")
patch_proto = loc_protos.Patch()
patch_proto.ParseFromString(f.read())
for matching_costs_file in matching_costs_files:
f = open(matching_costs_file, "rb")
matching_costs_proto = loc_protos.MatchingCosts()
matching_costs_proto.ParseFromString(f.read())
f.close()
mask.extend(patch_proto.elements)
mask.extend(matching_costs_proto.elements)
return mask
3 changes: 1 addition & 2 deletions src/python/visualize_localization_result.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@


def create_combined_image(matching_result, cost_matrix, expanded_mask=None):

rgb_costs = np.zeros((cost_matrix.shape[0], cost_matrix.shape[1], 3))
rgb_costs[:, :, 0] = cost_matrix
rgb_costs[:, :, 1] = cost_matrix
Expand Down Expand Up @@ -48,7 +47,7 @@ def main():
"--expanded_patches_dir",
required=False,
type=Path,
help="Path to directory with expanded nodes files of type .Patch.pb",
help="Path to directory with expanded nodes files of type .MatchingCosts.pb",
)
parser.add_argument(
"--image_name",
Expand Down
Loading
Loading