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.

Sign up for GitHub

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

Sort VectorValues::values() by key when TBB is ON #1740

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Sort VectorValues::values() by key when TBB is ON #1740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 14 additions & 8 deletions gtsam/linear/VectorValues.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ namespace gtsam {
}
}

/* ************************************************************************ */
std::map<Key, Vector> VectorValues::sorted() const {
std::map<Key, Vector> ordered;
for (const auto& kv : *this) ordered.emplace(kv);
Copy link

Choose a reason for hiding this comment

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

why not just use a std::vector<std::pair<Key, Vector>>, then performs reserve, emplace_back and sort ? The map structure is not continuous in memory, and the insertion operation is also very time-consuming.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a great suggestion @tanzby, however the map is used to maintain the same structure as that provided and expected by TBB. I don't think we know for certain if changing to a std::vector would play nicely with TBB or introduce some subtle bugs.

If you are well versed in how TBB works and can make a PR for this, we would welcome it.

return ordered;
}

/* ************************************************************************ */
VectorValues VectorValues::Zero(const VectorValues& other)
{
Expand Down Expand Up @@ -130,11 +137,7 @@ namespace gtsam {
GTSAM_EXPORT std::ostream& operator<<(std::ostream& os, const VectorValues& v) {
// Change print depending on whether we are using TBB
#ifdef GTSAM_USE_TBB
std::map<Key, Vector> sorted;
for (const auto& [key, value] : v) {
sorted.emplace(key, value);
}
for (const auto& [key, value] : sorted)
for (const auto& [key, value] : v.sorted())
#else
for (const auto& [key,value] : v)
#endif
Expand Down Expand Up @@ -176,7 +179,12 @@ namespace gtsam {
// Copy vectors
Vector result(totalDim);
DenseIndex pos = 0;
#ifdef GTSAM_USE_TBB
// TBB uses un-ordered map, so inefficiently order them:
for (const auto& [key, value] : sorted()) {
#else
for (const auto& [key, value] : *this) {
#endif
result.segment(pos, value.size()) = value;
pos += value.size();
}
Expand Down Expand Up @@ -392,9 +400,7 @@ namespace gtsam {
// Print out all rows.
#ifdef GTSAM_USE_TBB
// TBB uses un-ordered map, so inefficiently order them:
std::map<Key, Vector> ordered;
for (const auto& kv : *this) ordered.emplace(kv);
for (const auto& kv : ordered) {
for (const auto& kv : sorted()) {
#else
for (const auto& kv : *this) {
#endif
Expand Down
3 changes: 3 additions & 0 deletions gtsam/linear/VectorValues.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ namespace gtsam {
typedef ConcurrentMap<Key, Vector> Values; ///< Collection of Vectors making up a VectorValues
Values values_; ///< Vectors making up this VectorValues

/** Sort by key (primarily for use with TBB, which uses an unordered map)*/
std::map<Key, Vector> sorted() const;

public:
typedef Values::iterator iterator; ///< Iterator over vector values
typedef Values::const_iterator const_iterator; ///< Const iterator over vector values
Expand Down
Loading