Skip to content

Conversation

@aritorto
Copy link
Member

@aritorto aritorto commented Nov 7, 2023

When a CpGrid has LGRs, a Cartesian index (key) is associated to more than one compressed index (value). Therefore, this unordered_map now has been replaced by an unordered_multimap, to support multiple value with a same key.

@aritorto
Copy link
Member Author

aritorto commented Nov 7, 2023

jenkins build this serial please

Copy link
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Just a couple of minor points now, as an initial review. I haven't thought through the full implications of using using unordered_multimap<> here. Another option might be
unordered_map<int,std::vector<int>> instead.

Comment on lines 39 to 49
retval.reserve(num_cells);
if (global_cell) {
for (int i = 0; i < num_cells; ++i) {
retval.insert(std::pair<int,int>(global_cell[i], i));
}
} else {
for (int i = 0; i < num_cells; ++i) {
retval.insert(std::pair<int,int>(i, i));
}
return retval;
}
return retval;
Copy link
Member

Choose a reason for hiding this comment

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

If you're changing this anyway, now would be a good time to switch to

retval.emplace(x, y)

instead of

retval.insert(std::pair<int,int>(x, y))

They do the same thing, effectively, but the former (.emplace()) is (slightly) faster since it constructs the value in-place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your feedback! Indeed, unordered_map<int, std::vector> is an alternative too. I'll check it out.

Comment on lines 41 to 44
for (const auto pair : cartesian_to_compressed)
{
std::cout << pair.first << " " << pair.second << '\n';
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this code left over from development? If so, I think we should remove it.

Copy link
Member Author

@aritorto aritorto Nov 7, 2023

Choose a reason for hiding this comment

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

Opps! Yes, that was supposed to be removed. Thanks for noticing!

@aritorto aritorto force-pushed the cartesianToCompressedMulti branch from 94769bd to db56d39 Compare November 7, 2023 11:28
@aritorto aritorto marked this pull request as draft November 7, 2023 11:39
@aritorto aritorto added the manual:irrelevant This PR is a minor fix and should not appear in the manual label May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:irrelevant This PR is a minor fix and should not appear in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants