2012/7/31 Timon Gehr <timon.gehr@gmx.ch>:
> I realize that the code is just for fun, but I have some comments:
>
> - don't .dup stuff just in order to index directly. it never has any effect other than performance degradation. (this could be a compiler warning)
>
> - crossOver and mutate mutate their parameters in-place. I assume this is by mistake. reproduce effectively inserts every element a couple times because of this -- this is why the bug manifests reliably. (const, immutable annotations?)
>
> - make use of $
>
> int from=uniform(0,victim.dna.length);
> int to=uniform(0,victim.dna.length);
> swap(victim.dna[from],victim.dna[to]);
>
> =>
>
> swap(victim.dna[uniform(0,$)],
>      victim.dna[uniform(0,$)]);
>
> - sort is in-place
>
> workers=array(sort!(fitnessCompare)(workers));
>
> =>
>
> sort!fitnessCompare(workers)
>
> - mark local functions static if they do not need to access the enclosing context.
>
> - use size_t for array iteration variables (if necessary)
>
>
> for(int x=0;x<cities.length-1;x++)
>     travelled+=distance(cities[x],cities[x+1]);
>
> =>
>
> for(size_t x=1;x<cities.length;x++)
>     travelled+=distance(cities[x-1],cities[x]);
>
> I also removed the subtraction from the array length. This would be correct in this case because cities.length>=2 is guaranteed, but it is an error prone pattern.
>
> - another way to do it is to drop the loop completely and to use ranges instead:
>
> return zip(cities,cities.drop(1))
>       .map!(a=>distance(a.expand))
>       .reduce!"a+b";
>
>
> The benefit is that this is now obviously correct.
>
>
> You might also want to consider not building up the cities array and computing the terms at the boundary explicitly instead:
>
> return distance(city(0,0), victim.dna[0]) +
>        distance(victim.dna[$-1], city(0,0)) +
>        zip(victim.dna, victim.dna.drop(1))
>       .map!(a=>distance(a.expand))
>       .reduce!"a+b";
>
> The goal is to craft the shortest code possible that is still efficient and easy to follow.
>
> - type deduction?
>
>
> further comments whose application does not lead to immediate benefit:
>
> - in contracts are better specified in their dedicated section to push the requirements onto the caller.
>
> - consider for(;;) as a means for indefinite looping.
>
> - the convention is upper case for type names

Thank you very much for this criticism, I'm relatively new to programming and these mistakes/points are the kind off things you can't learn from reading a book or two.

I have one little question about one of the last points though
why use for(;;)?
As far as I can tell this simply reduces readability from while(true).
Is there any reason for this?

On 07/31/2012 12:30 AM, maarten van damme wrote:
> 2012/7/31 Timon Gehr<timon.gehr@gmx.ch>:
>> ...
>> further comments whose application does not lead to immediate benefit:
>>
>> - in contracts are better specified in their dedicated section to push
>> the requirements onto the caller.
>>
>> - consider for(;;) as a means for indefinite looping.
>>
>> - the convention is upper case for type names
>
> Thank you very much for this criticism, I'm relatively new to
> programming and these mistakes/points are the kind off things you
> can't learn from reading a book or two.
>

I'm glad to help.

> I have one little question about one of the last points though
> why use for(;;)?

Well, as stated, there is no benefit.

> As far as I can tell this simply reduces readability from while(true).

That is entirely a matter of preference.

> Is there any reason for this?

I like it more because it says "loop".
while(true) says: "loop as long as 'true' still holds", which is silly.

On 7/31/12, Timon Gehr <timon.gehr@gmx.ch> wrote:
> I like it more because it says "loop".

I'd love to have "loop { }". But keyword bloat yada yada. :)

Jonathan M Davis:

> I'd actually argue that structs without opCmp shouldn't have it implicitly defined. It's just begging for bugs otherwise.

For reference, this is the issue we are talking about:
http://d.puremagic.com/issues/show_bug.cgi?id=3789

The current situation is totally not acceptable. Even doing what I think you are suggesting, that is generating a compile-time error, is better than the current situation, that leads to silent bugs.

Bye,
bearophile

On Monday, 30 July 2012 at 22:58:28 UTC, Timon Gehr wrote:
> On 07/31/2012 12:30 AM, maarten van damme wrote:
>> 2012/7/31 Timon Gehr<timon.gehr@gmx.ch>:
>>> ...
>>> further comments whose application does not lead to immediate benefit:
>>>
>>> - in contracts are better specified in their dedicated section to push
>>> the requirements onto the caller.
>>>
>>> - consider for(;;) as a means for indefinite looping.
>>>
>>> - the convention is upper case for type names
>>
>> Thank you very much for this criticism, I'm relatively new to
>> programming and these mistakes/points are the kind off things you
>> can't learn from reading a book or two.
>>
>
> I'm glad to help.
>
>> I have one little question about one of the last points though
>> why use for(;;)?
>
> Well, as stated, there is no benefit.
>
>> As far as I can tell this simply reduces readability from while(true).
>
> That is entirely a matter of preference.
>
>> Is there any reason for this?
>
> I like it more because it says "loop".
> while(true) says: "loop as long as 'true' still holds", which is silly.

I've usually see the "for(;;)" syntax to loop infinity: This reads as: Just keep looping, with no condition.

On Tuesday, 31 July 2012 at 00:21:59 UTC, bearophile wrote:
> Jonathan M Davis:
>
>> I'd actually argue that structs without opCmp shouldn't have it implicitly defined. It's just begging for bugs otherwise.
>
> For reference, this is the issue we are talking about:
> http://d.puremagic.com/issues/show_bug.cgi?id=3789
>
> The current situation is totally not acceptable. Even doing what I think you are suggesting, that is generating a compile-time error, is better than the current situation, that leads to silent bugs.
>
> Bye,
> bearophile

Oh, thanks. Because we were talking about sorting, and you spoke of "comparison", I didn't get you meant comparison in the opEquals way (as opposed to equivalence).

Thanks for taking the time to look for the bug for me.

On Monday, 30 July 2012 at 22:23:10 UTC, Timon Gehr wrote:
> On 07/30/2012 03:52 PM, monarch_dodra wrote:
>> ...
>> NaN then compares false with everything, making it un-transitive, and
>> potentially breaking your cmp. COuld you try the algo with "return
>> 1/(1+travelled)".
>>
>> That, or because of the non-deterministic nature  of floating point
>> calculation,
>
> Floating-point computation is deterministic.

Is this specific to D? Because it isn't in C++:

http://www.parashift.com/c++-faq-lite/floating-point-arith2.html

I now tried to bring it to the next level, using concurrency to bread
a couple of populations and make the best solutions migrate between
them.
But, to use concurrency in D, one has to continually cast between
immutable, cast immutable away, ....
Not only that, casting away from immutable (or too, the error message
is not informative at all) you get error's along the line of "opEquals
doesn't work with immutable arguments"...

Why is it that cumbersome? It seems like D took a good idea and ruined it with all that const-stuff.

I have the feeling that I've completely missed the whole idea on how
concurrency in D should work. Is it normal that you have to either
mess with immutable or with the broken shared?
Am I implementing everything wrong?

On Tuesday, July 31, 2012 11:41:19 maarten van damme wrote:
> I now tried to bring it to the next level, using concurrency to bread
> a couple of populations and make the best solutions migrate between
> them.
> But, to use concurrency in D, one has to continually cast between
> immutable, cast immutable away, ....
> Not only that, casting away from immutable (or too, the error message
> is not informative at all) you get error's along the line of "opEquals
> doesn't work with immutable arguments"...
> 
> Why is it that cumbersome? It seems like D took a good idea and ruined it with all that const-stuff.
> 
> I have the feeling that I've completely missed the whole idea on how
> concurrency in D should work. Is it normal that you have to either
> mess with immutable or with the broken shared?
> Am I implementing everything wrong?

To pass something across threads, it either needs to be a value type, or shared, or immutable. Unfortunately, shared doesn't work std.concurrency right now though, so you're stuck using immutable. What we really need is a way to indicate that ownership is being passed from one thread to another, but that can't really be done with the current type system. However, there are probably tweaks that can be made to make it more pleasant to deal with.

As for "messing it up with const", immutability (and therefore const) is a core part of D's threading model. By making everything thread-local by default, you then need a way to share instances across threads, and that requires shared (and immutable is implicitly shared). And with immutable, that can be done much more efficiently, because then the compiler doesn't have to worry about multiple threads screwing with the variable.

So, the situation isn't perfect and still needs some work, but the basic idea is solid and does work, even if it deosn't yet work quite as well as we'd like.

- Jonathan M Davis

On Tuesday, 31 July 2012 at 09:41:31 UTC, maarten van damme wrote:
> I now tried to bring it to the next level, using concurrency to bread
> a couple of populations and make the best solutions migrate between
> them.
> But, to use concurrency in D, one has to continually cast between
> immutable, cast immutable away, ....
> Not only that, casting away from immutable (or too, the error message
> is not informative at all) you get error's along the line of "opEquals
> doesn't work with immutable arguments"...
>
> Why is it that cumbersome? It seems like D took a good idea and ruined
> it with all that const-stuff.
>
> I have the feeling that I've completely missed the whole idea on how
> concurrency in D should work. Is it normal that you have to either
> mess with immutable or with the broken shared?
> Am I implementing everything wrong?

Use std.parallelism. It has everything that (I think) you need, e.g. support for parallel for, tasks and more.