Return to Answer

added 1246 characters in body

Source Link

edited May 10, 2017 at 13:52

24.2k
1
48
93

Note that we've now called two methods which iterate partially or fully through values. If it was a non-deterministic iterator we have problems. An example of a non-deterministic iterator would be

public IEnumerable<int> Demo()
{
    var rnd = new Random();
    for (int i = rnd.Next(10); i > 0; i--) yield return rnd.Next();
}

If you call this once and then loop through the resulting iterator multiple times then it will not consistently return the same values. You may think that in real usage you won't come across iterators like that, but sometimes the non-determinism is a lot more subtle.

Finally, on the names. In your context it might be obvious that Sample means "sample without replacement", but in some contexts it might be important to distinguish between sampling with and without replacement. Consider whether you might need to distinguish the cases in future usage of the library.

Postscript: t3chb0t's suggestion of a Randomise method which can then be combined with Take is compatible with a rewrite of your GetSamples method, and in fact this is what I have kicking around in my library of utility methods (although I call it Shuffle). As a modification of your code:

    public static IEnumerable<T> Randomise<T>(IEnumerable<T> values, Random rnd = null)
    {
        if (rnd == null) rnd = _rnd;

        var source = new List<T>(values);
        while (source.Count > 0)
        {
            var index = rnd.Next(source.Count);
            yield return source[index];
            source[index] = source[source.Count - 1];
            source.RemoveAt(source.Count - 1);
        }
    }

Note that we've now called two methods which iterate partially or fully through values. If it was a non-deterministic iterator we have problems.

Note that we've now called two methods which iterate partially or fully through values. If it was a non-deterministic iterator we have problems. An example of a non-deterministic iterator would be

public IEnumerable<int> Demo()
{
    var rnd = new Random();
    for (int i = rnd.Next(10); i > 0; i--) yield return rnd.Next();
}

    public static IEnumerable<T> Randomise<T>(IEnumerable<T> values, Random rnd = null)
    {
        if (rnd == null) rnd = _rnd;

        var source = new List<T>(values);
        while (source.Count > 0)
        {
            var index = rnd.Next(source.Count);
            yield return source[index];
            source[index] = source[source.Count - 1];
            source.RemoveAt(source.Count - 1);
        }
    }

Source Link

created May 10, 2017 at 11:35

Peter Taylor

24.2k
1
48
93

        public static T Sample<T>(this IEnumerable<T> values)

        {
            return Sample(values, 1).First();
        }

Nice helper method.

            if(!values.Any())

            {
                throw new InvalidOperationException("Collection empty.");
            }

I would have used an ArgumentOutOfRangeException, but that's a matter of personal preference. Should it throw an exception if size == 0 or return an empty list? That's a tricky edge case which should be clearly documented.

        public static IEnumerable<T> Sample<T>(this IEnumerable<T> values, int size)

I would prefer the Random to be an argument too, even if it's as Random rnd = null with a fallback inside the method. Being able to seed your random selection is important for testing the code and for reproducing results if it's a scientific project.

            if (size < 1)

            {
                return new List<T>();
            }

I would definitely prefer to throw an ArgumentOutOfRangeException if size < 0.

            if (size >= values.Count())

            {
                return values;
            }

If size > values.Count() then I think that should be ArgumentOutOfRangeException or InvalidOperationException. Again, whichever way you finally decide, it should be clearly documented.

Note that we've now called two methods which iterate partially or fully through values. If it was a non-deterministic iterator we have problems.

            var samples = new List<T>();

            var source = new List<T>(values);

Since we have to convert source to a list here, there's an argument for doing it earlier and ensuring that we only iterate through values once, solving the problem of non-deterministic iterators.

                source.Remove(value);

This is expensive. It's asymptotically more efficient to replace this with

                source[index] = source[source.Count - 1];
                source.RemoveAt(source.Count - 1);