refactor: Remove Configuration AtomTypeMix - #2220
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 6edd5d6 | Previous: 41d9664 | Ratio |
|---|---|---|---|
BM_Box_MinimumDistance<TriclinicBox> |
24.659513680822435 ns/iter |
11.082351824938764 ns/iter |
2.23 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @disorderedmaterials/dissolve-devs
RobBuchananCompPhys
left a comment
RobBuchananCompPhys
left a comment
There was a problem hiding this comment.
Looks good 👍
| [&](int indexI, const auto &popI, int indexJ, const auto &popJ) | ||
| { | ||
| auto key = DoubleKeyedMapKey{atd1.atomTypeName(), atd2.atomTypeName()}; | ||
| auto key = DoubleKeyedMapKey{popI.first->name(), popJ.first->name()}; |
There was a problem hiding this comment.
On one hand I do quite like the idea of self-contained class we can instantiate to store these more abstract keys - however, I worry it is a bit onerous to fully initialise a key in this way. Is it not possible to overload an operator on DoubleKeyedMap to handle it all-in-one?
|
|
||
| auto &bound = unweightedsq.boundPartials().get(key); | ||
| auto &unbound = unweightedsq.unboundPartials().get(key); | ||
| auto &partial = unweightedsq.partials().get(key); |
There was a problem hiding this comment.
Further to the previous comment, is:
auto &bound = unweightedsq.boundPartials[popI.first->name(), popJ.first->name()] where & operator[]
not possible?
There was a problem hiding this comment.
The answer is "Yes, but...". You could absolutely have an operator[] which takes a std::pair of the names and does the lookup (you can't have multiple args to operator[] so you have to wrap them in something like a std::pair). The "but" is that we typically only want to use it as a const getter, while std::map's operator[] is fully non-const as it creates new values if the key is not present. So, in the interest of maintaining "semantic compatibility" between DoubleKeyedMap and std::map, I elected to implement get() and at() instead.
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
2 participants
Follows #2214
This PR removes the
AtomTypeMixfromConfiguration, moving towards a dynamic calculation of atom type populations etc. from storedSpeciespopulations. The effects are quite wide-ranging, so this is the only thing addressed here.