Jens Mueller:

> Which makes me think Phobos is convenient enough in this use case.

I don't agree. This is not quite worse
auto randomLetter = () => randomSample(letters, 1, letters.length).front;

than:
auto randomLetter = () => letters.choice;

Bye,
bearophile

bearophile wrote:
> Jens Mueller:
> 
> >Which makes me think Phobos is convenient enough in this use case.
> 
> I don't agree. This is not quite worse
> auto randomLetter = () => randomSample(letters, 1,
> letters.length).front;
> 
> than:
> auto randomLetter = () => letters.choice;

Then add choice explicitly.
auto choice(R)(R r) { return randomSample(r, 1, r.length).front; };

The point is that choice is a one-liner when using randomSample. It's a special case of randomSample. But you may argue that the use case occurs that often that it should be added. Don't know whether this is the case.

Jens

On Saturday, 23 February 2013 at 13:02:38 UTC, Jens Mueller wrote:
>
> I think this version also looks good:
> void main()
> {
> 	auto randomLetter = () => randomSample(letters, 1, letters.length).front;
> 	writeln(iota(10).map!(_ => randomLetter()));
> }
>
> Which makes me think Phobos is convenient enough in this use case. Just
> finding a simple way to use it can be a burden. It should be improved
> with better documentation.
>
> Jens

No offense, but I think that's horrible. No matter how you put it, you should keep in mind that the strength of these ranges is being able do stuff lazily.

The final product of what you have is a horrible contraption of chained ranges, that go out of their way just to pick a random char for you.

Not to mention, you are creating a randomSample per character, which is simply not the way it is designed to be used. Generating an entire new range just to generate a random letter...? At least: use uniform instead:

auto randomLetter = () => letters[uniform (0, letters.length);

Still, even with that, IMO, the end product is bug prone, not very clear, and horribly inefficient. I'd hate to be the one to have to maintain this down the pipe... And at the end of the day, you'd still need to add an "array" at the end if you want to put it in a string anyways.

--------
Honestly, what happened to doing it by hand? It's more robust, cleaner, easy to understand...


    string randomString;
    {
        auto app = Appender!string();
        foreach(_ ; 0 .. 10)
            app.put(letters[uniform(0, letters.length)]);
        randomString = app.data;
    }

This is the "easy and safe" version. You could make it even more simple with a pre-allocate dchar:


    string randomString;
    {
        auto tmp = new dchar[](10);
        foreach(ref c ; tmp)
            c = letters[uniform(0, letters.length)];
        randomString = tmp.to!string();
    }

Or, if you know your letters are all ascii, it gets even more simple and efficient:

    string randomString;
    {
        auto tmp = new char[](10);
        foreach(ref char c ; tmp)
            c = letters[uniform(0, letters.length)];
        randomString = cast(string)letters;
    }

All those scope blocks I put in are not mandatory. At the end of the day, it is 4 lines of code. Very efficient, and the intent is crystal clear.

I know it's not very exciting code, they do say good code is boring code.

monarch_dodra wrote:
> On Saturday, 23 February 2013 at 13:02:38 UTC, Jens Mueller wrote:
> >
> >I think this version also looks good:
> >void main()
> >{
> >	auto randomLetter = () => randomSample(letters, 1,
> >letters.length).front;
> >	writeln(iota(10).map!(_ => randomLetter()));
> >}
> >
> >Which makes me think Phobos is convenient enough in this use case.
> >Just
> >finding a simple way to use it can be a burden. It should be
> >improved
> >with better documentation.
> >
> >Jens
> 
> No offense, but I think that's horrible. No matter how you put it, you should keep in mind that the strength of these ranges is being able do stuff lazily.
> 
> The final product of what you have is a horrible contraption of chained ranges, that go out of their way just to pick a random char for you.
> 
> Not to mention, you are creating a randomSample per character, which is simply not the way it is designed to be used. Generating an entire new range just to generate a random letter...? At least: use uniform instead:
> 
> auto randomLetter = () => letters[uniform (0, letters.length);
> 
> Still, even with that, IMO, the end product is bug prone, not very clear, and horribly inefficient. I'd hate to be the one to have to maintain this down the pipe... And at the end of the day, you'd still need to add an "array" at the end if you want to put it in a string anyways.
> 
> --------
> Honestly, what happened to doing it by hand? It's more robust, cleaner, easy to understand...
> 
> 
>     string randomString;
>     {
>         auto app = Appender!string();
>         foreach(_ ; 0 .. 10)
>             app.put(letters[uniform(0, letters.length)]);
>         randomString = app.data;
>     }
> 
> This is the "easy and safe" version. You could make it even more simple with a pre-allocate dchar:
> 
> 
>     string randomString;
>     {
>         auto tmp = new dchar[](10);
>         foreach(ref c ; tmp)
>             c = letters[uniform(0, letters.length)];
>         randomString = tmp.to!string();
>     }
> 
> Or, if you know your letters are all ascii, it gets even more simple and efficient:
> 
>     string randomString;
>     {
>         auto tmp = new char[](10);
>         foreach(ref char c ; tmp)
>             c = letters[uniform(0, letters.length)];
>         randomString = cast(string)letters;
>     }
> 
> All those scope blocks I put in are not mandatory. At the end of the day, it is 4 lines of code. Very efficient, and the intent is crystal clear.
> 
> I know it's not very exciting code, they do say good code is boring code.

I see your point. But I believe it depends on what you want to achieve
and how you are constrained. I do not care about very good efficiency in
this case. I just need a solution that works. I can replace that code
later when needed. Your solution is less precise but much more
efficient.
Can we have a generic function/code that is as efficient as yours? That
is what we are aiming for, right?

My hope is that machine code generated from

auto randomLetter = () => letters[uniform (0, letters.length)];
auto randomString = iota(10).map!(_ => randomLetter()).array();

is on par with hand-written code most of the time. Or at least it's fast enough for many use cases.

Jens

Jens Mueller:

> It's a special case of randomSample.

Maybe in theory, but I am not going to use randomSample to generate a random string. Your code is slow and bug-prone.


> But you may argue that the use case occurs
> that often that it should be added.

Right. (I even argue that in Phobos today there are functions that are much less commonly useful than a choice()).

Bye,
bearophile

On Saturday, 23 February 2013 at 14:28:28 UTC, Jens Mueller wrote:
> Can we have a generic function/code that is as efficient as yours?

Well, for now, we have "map" (and "iota"). This usually covers enough ground, but there remains cases where it is sub-optimal.

What we would need is a "generate". Basically, a range that calls a function to generate the value of front on the fly. I've seen enough threads like this one that "abuse" iota/repeat/take/map (or sequence and recurrence for that matter), just to obtain a generic generator.

Let's just write Generator/generator.

On 2/23/13 4:25 PM, Jens Mueller wrote:
> auto randomLetter = () =>  letters[uniform (0, letters.length)];
> auto randomString = iota(10).map!(_ =>  randomLetter()).array();

auto randomString = iota(10).map!(_ => letters[uniform(0, $)]).array;

Bearophile has long asked for a one-parameter uniform(n) that does what uniform(0, n) does. Perhaps this provides a good supporting argument. With that enhancement:

auto randomString = iota(10).map!(_ => letters[uniform($)]).array;

I expect this to be fast and only allocate once.


Andrei

On 2/23/13 5:56 PM, monarch_dodra wrote:
> On Saturday, 23 February 2013 at 14:28:28 UTC, Jens Mueller wrote:
>> Can we have a generic function/code that is as efficient as yours?
>
> Well, for now, we have "map" (and "iota"). This usually covers enough
> ground, but there remains cases where it is sub-optimal.
>
> What we would need is a "generate". Basically, a range that calls a
> function to generate the value of front on the fly. I've seen enough
> threads like this one that "abuse" iota/repeat/take/map (or sequence and
> recurrence for that matter), just to obtain a generic generator.
>
> Let's just write Generator/generator.

Make sure you copy Sequence/sequence :o).

Andrei

On Saturday, 23 February 2013 at 16:25:45 UTC, Andrei Alexandrescu wrote:
> On 2/23/13 4:25 PM, Jens Mueller wrote:
>> auto randomLetter = () =>  letters[uniform (0, letters.length)];
>> auto randomString = iota(10).map!(_ =>  randomLetter()).array();
>
> auto randomString = iota(10).map!(_ => letters[uniform(0, $)]).array;
>
> Bearophile has long asked for a one-parameter uniform(n) that does what uniform(0, n) does. Perhaps this provides a good supporting argument. With that enhancement:
>
> auto randomString = iota(10).map!(_ => letters[uniform($)]).array;
>
> I expect this to be fast and only allocate once.
>
>
> Andrei

I'm still not a fan of the iota+map combo.

I wrote this (Fast)Generator template. The generic "generator" bookkeeps the calls to front and popFront.

The "fast" variant assumes that you'll basically just call front+popFront+front+popFront, or that you don't about bookeeping.

This can be interesting, as often, iota+map is straight up linearly consumed.

//----
import std.stdio, std.range, std.conv, std.random;

template zeroaryFun(alias fun)
{
    static if (is(typeof(fun) : string))
    {
        auto zeroaryFun()
        {
            mixin("return (" ~ fun ~ ");");
        }
    }
    else
    {
        alias fun zeroaryFun;
    }
}

struct FastGenerator(alias F)
{
    private alias fun = zeroaryFun!F;
    private alias T = typeof(fun());

    enum empty = false;
    @property auto front() {return fun();}
    void popFront() {}
}
struct Generator(alias F)
{
    private alias fun = zeroaryFun!F;
    private alias T = typeof(fun());
    private T cache;
    private bool called = false;

    enum empty = false;

    @property
    auto front()
    {
        if (!called)
        {
            cache = fun();
            called = true;
        }
        return cache;
    }

    void popFront()
    {
        if (!called)
            fun();
        called = false;
    }
}

void main()
{
    dstring letters = "abcd";
    int i = 0;
    writeln(FastGenerator!(() => (i++))().take(9)); //Who needs iota?
    writeln(FastGenerator!"'s'"().take(5)); //Who needs repeat?
    writeln(FastGenerator!(() => letters[uniform(0, $)])().take(9)); //Simple,clear,concise.
}
//----

In these cases, Generator is something that is both convenient and fast.

I realize it's not an incredibly higher order function or anything, but I wouldn't mind having something like this in Phobos. It would just make every day coding that much more convenient and simple.

On Saturday, 23 February 2013 at 16:29:49 UTC, Andrei Alexandrescu wrote:
> On 2/23/13 5:56 PM, monarch_dodra wrote:
>> On Saturday, 23 February 2013 at 14:28:28 UTC, Jens Mueller wrote:
>>> Can we have a generic function/code that is as efficient as yours?
>>
>> Well, for now, we have "map" (and "iota"). This usually covers enough
>> ground, but there remains cases where it is sub-optimal.
>>
>> What we would need is a "generate". Basically, a range that calls a
>> function to generate the value of front on the fly. I've seen enough
>> threads like this one that "abuse" iota/repeat/take/map (or sequence and
>> recurrence for that matter), just to obtain a generic generator.
>>
>> Let's just write Generator/generator.
>
> Make sure you copy Sequence/sequence :o).
>
> Andrei

Have you actually tried it? Do you know what the signature of a function passed to sequence is?

I think this bug entry sums it up pretty well, and why "Sequence" is not really adapted:

http://forum.dlang.org/thread/bug-9550-3@http.d.puremagic.com%2Fissues%2F

//----
import std.stdio, std.range, std.conv, std.random, std.typecons;

void main()
{
    dstring letters = "abcd";
    dchar fun(Tuple!(), uint) {return letters[uniform(0, $)];}
    writeln(sequence!fun().take(9));
}
//----

I failed to use a lambda or mixin in this case.

I really don't think we should push to try to warp the existing stuff to work in any scenario, but rather, give the tools required to work intuitively for any scenario.