Revise "Add block_ids_from_names"#7085
Conversation
nilsvu
force-pushed
the
block_ids_from_names
branch
3 times, most recently
from
3ba156b to
e2e1d66
Compare
nilsdeppe
left a comment
nilsdeppe
left a comment
There was a problem hiding this comment.
LGTM, a few small suggestions that you can squash immediately. That's for doing this :)
src/Domain/Structure/BlockGroups.hpp
Outdated
| block_groups); | ||
|
|
||
| /*! | ||
| * \brief Get the block IDs corresponding to the given block or group names. |
There was a problem hiding this comment.
maybe add sorted block IDs? Then people can rely on it being sorted.
There was a problem hiding this comment.
That's a good point. Probably with fairly clear instructions it should do well. Given this is pretty deterministic and at least Claude can use clangd, this could work
| "The block or group 'NoBlock' is not one of the block names or " | ||
| "groups of the domain.")); | ||
| CHECK(block_ids_from_names({"Block1", "Group2"}, all_block_names, | ||
| block_groups) == std::vector<size_t>{0, 2, 3}); |
There was a problem hiding this comment.
Could you add a test that explicitly verifies the data is sorted on output? I think this one sort of does, but it might be worth doing {"Block2", "Block1"} and checking it is sorted and that it is the same as {"Block1", "Block2"}.
Can you also test you get the expected output for {"Block1", "Group1"}?
|
|
||
| /*! | ||
| * \brief Get the block IDs corresponding to the given block or group names. | ||
| * |
There was a problem hiding this comment.
Document that this will throw an exception if a block or group name is not present?
| } | ||
| // Sort the block IDs just so they're easier to deal with. | ||
| alg::sort(only_dg_block_ids_.value()); | ||
| only_dg_block_ids_ = domain::block_ids_from_names( |
There was a problem hiding this comment.
[optional but appreciated] I think the header file or whenever the binary_search is should also be switched to the linear search since that'll be faster for such small data.
| * \param block_groups Block groups used to expand the names. | ||
| * \return std::vector<size_t> Sorted list of block IDs. | ||
| */ | ||
| std::vector<size_t> block_ids_from_names( |
There was a problem hiding this comment.
I'm undecided on this, but I wonder if we should in general switch to uint16_t for block IDs. I think we could even do so locally in places without worrying about it. It seems unlikely we will have ~2^16 blocks any time soon. The main advantage is reduced size and possibly faster lookup. This is going to be such a small cost that I don't think we should put extra effort into changing it, but if we decide it's a reasonable direction to go we could do so gradually as we remember 🤷 To be clear, I'm not asking for a change in this PR, just wanted to mention it.
There was a problem hiding this comment.
We also have the class BlockId that's used in some old code. We could revive that and use it instead of size_t, then we can swap out the underlying representation if we want. A coding agent might be able to do the refactor.
|
Pushed a fixup and another commit for the linear search |
nilsvu
force-pushed
the
block_ids_from_names
branch
from
90262a4 to
1263883
Compare
|
LGTM! Please squash! And thanks for doing this :) |
This reverts commit 883964a.
Implemented as std::vector for fast lookup.
nilsvu
force-pushed
the
block_ids_from_names
branch
from
1263883 to
d674a85
Compare
|
Rebased and squashed 👍 |
2 participants
Proposed changes
Switch from std::set to std::vector as discussed in #7061.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments