Sort VectorValues::values() by key when TBB is ON #1740
Conversation
|
@dellaert Hmm, the Python CI with TBB timed out after 360 minutes. Should I try it again? |
|
@arutkowski can you please merge in the latest develop? I upgraded the OS on the Python CI to tackle this time out issue and we haven't been seeing it since. |
|
@varunagrawal It looks like I'm up to date now (1 commit ahead of 11409b0). It also looks like it passed CI. |
| /* ************************************************************************ */ | ||
| std::map<Key, Vector> VectorValues::sorted() const { | ||
| std::map<Key, Vector> ordered; | ||
| for (const auto& kv : *this) ordered.emplace(kv); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR fixes #1731 as the title suggests.
Specifically, a private member function
sorted()was added, which is used byvalues(),operator<<, andhtml(const KeyFormatter&)to sort the output by key as a user would expect.