Implement weighted random number generation using maps #44
Conversation
| struct is_map : public std::false_type {}; | ||
|
|
||
| template<typename Type> | ||
| struct is_map<Type, void_t<typename Type::key_type, typename Type::mapped_type, typename Type::value_type>> : public std::true_type{}; |
| MappedType random_weight = get(0, total_weight - 1); | ||
| MappedType sum = 0; | ||
|
|
||
| for(IteratorType it = std::begin(map_container); it != std::end(map_container); ++it) |
There was a problem hiding this comment.
By iterating through map we're missing reason of using the map instead of vector<pair<Key, Value>>
And for unordered map the order of weights will be different and result of this function will be different
I'm not expert in this algorithm, can it be done with map::upper_bound or map::lower_bound function ?
https://en.cppreference.com/w/cpp/container/map/upper_bound
for logarithmic complexity ?
There was a problem hiding this comment.
The upper_bound and lower_bound functions operate on the keys of the map so, it is not suitable for this case. And even if it operated on values, we had to store total weight until that object instead of the weight of that object as the value.
Also, the "ordering" of std::unordered_map should not affect the result. Because the probability of each item getting picked is directly tied to its weight.
There was a problem hiding this comment.
Fine, can you code a simple test case for this inside test/random_test.cpp file ?
I can do it by myself but later next week
There was a problem hiding this comment.
During testing, I found out the algorithm fails for floats and doubles as values. I am also fixing that. Also, do you have specific test case(s) that you want me to code.
There was a problem hiding this comment.
I have nothing special, you can try your best
There was a problem hiding this comment.
All done. But I have one question, should I check whether if there is a negative weight using
assert?
It's much better to not allow users to use negative values at all by disallowing it with extra condition for enable_if
Something like this:
!std::is_signed<MapContainer::value_type >::value
And compilation should fail with signed map value types, so users required to use only unsigned types
There was a problem hiding this comment.
For real numbers (float, double, long double) assert is ok
There was a problem hiding this comment.
But this completely disallows users to use reals as weights.
There was a problem hiding this comment.
std::is_unsigned<MapContainer::value_type>::value || is_uniform_real<MapContainer::value_type>::value
...
assert(std::is_unsigned<MapContainer::value_type>::value || map_has_all_positive_real_numbers())
include/effolkronium/random.hpp
Outdated
| details::is_map<MapContainer>::value && | ||
| details::is_iterator<decltype(std::begin(map_container))>::value && | ||
| (details::is_uniform_int<typename MapContainer::mapped_type>::value || details::is_byte<typename MapContainer::mapped_type>::value) && | ||
| std::is_same<Key, common>::value, |
There was a problem hiding this comment.
common type is reserved for common type randoms
I think it would be better to add another key type 'weight' for this feature
|
CI windows job failed. Do you have Windows env ? 1 error in CI: 2 error:
It because the min or max macro from Windows.h conflicts with the numeric_limit static method |
|
Now it should work. For some reason, I couldn't get |
|
@DolphyWind Hey, can you fix conflicts with latest merge ? Now you required to paste the new |
|
It should be ok now. Test have been passed. |
Closes #43