Version 2.0.0 (main 89d3166)
First of all, thanks for the great package.
I just started using NRandom in a unity project and noticed while using WeightedList.GetRandom that I always seem to get the last element of a list, not matter how often I call it.
Passing an IRandom or not and no matter which IRandom implementation i use, all returned elements are always the last of the list.
I also downloaded the NRandom source code and tried the unit tests in the c# solution.
All the tests do pass, but they don't seem to test anything concerning the distribution of results.
After adding some logging to WeightedListTests.Test_GetRandom it also shows that all returned elements are always the last one from the list.
Here is the code and the results from my test:
[Test]
public void Test_GetRandom()
{
var list = new WeightedList<int>
{
{ 1, 1.0 },
{ 2, 2.0 },
{ 3, 3.0 },
};
var distribution = new Dictionary<int, int>
{
{ 1, 0 },
{ 2, 0 },
{ 3, 0 }
};
for (int i = 0; i < 10000; i++)
{
var element = list.GetRandom();
Assert.That(list, Has.Count.EqualTo(3));
Assert.That(element, Is.EqualTo(1).Or.EqualTo(2).Or.EqualTo(3));
distribution[element]++;
}
Console.WriteLine("Random Distribution:");
foreach (var kvp in distribution)
{
Console.WriteLine($"{kvp.Key}: {kvp.Value}");
}
var empty = new WeightedList<int>();
Assert.Throws<InvalidOperationException>(() => empty.GetRandom());
}
The output is:
Standard Output:
Random Distribution:
1: 0
2: 0
3: 10000
After looking at it, there seem to actually be two issues in the new implementation of WeightedList.GetRandom
public void GetRandom(IRandom random, Span<T> destination)
{
if (list.Count == 0) throw new InvalidOperationException("Empty list");
for (int n = 0; n < destination.Length; n++)
{
var r = random.NextDouble() * totalWeight;
var current = 0.0;
for (int i = 0; i < list.Count; i++)
{
current += list[i].Weight;
if (r <= current)
{
destination[n] = list[i].Value;
continue;
}
}
destination[n] = list[^1].Value;
}
}
In the old GetItem function, the loop returned immediately when an element was matched.
Now the "continue" causes the loop to always run over all elements of the list and so always the last gets filled into destination[n].
I guess this was meant to be a "break"?
Also "destination[n] = list[^1].Value;" at the end then always overwrites the value again.
Version 2.0.0 (main 89d3166)
First of all, thanks for the great package.
I just started using NRandom in a unity project and noticed while using WeightedList.GetRandom that I always seem to get the last element of a list, not matter how often I call it.
Passing an IRandom or not and no matter which IRandom implementation i use, all returned elements are always the last of the list.
I also downloaded the NRandom source code and tried the unit tests in the c# solution.
All the tests do pass, but they don't seem to test anything concerning the distribution of results.
After adding some logging to WeightedListTests.Test_GetRandom it also shows that all returned elements are always the last one from the list.
Here is the code and the results from my test:
The output is:
After looking at it, there seem to actually be two issues in the new implementation of WeightedList.GetRandom
In the old GetItem function, the loop returned immediately when an element was matched.
Now the "continue" causes the loop to always run over all elements of the list and so always the last gets filled into destination[n].
I guess this was meant to be a "break"?
Also "destination[n] = list[^1].Value;" at the end then always overwrites the value again.