std.algorithm: {min,max}Element for a single range

[wilzbach]

resubmisson of #4019 - as extremum turned out not to be very useful, I changed it to accept directly the mapping function. This is now more efficient as the mapping function is called only once :)

@andralex has already approved the name:

Overall I think this is rather foreign compared to prevalent Phobos style. I think extremum,minElement, and maxElement with the appropriate predicates are the way to go.

Pinging @JackStouffer @burner @ntrel @klickverbot @JackStouffer

[JackStouffer]

immutable

[JackStouffer]

Needs another example of a custom accessor, something like the assert on line 1279

[JackStouffer]

This also needs tests using the dummy ranges, as you have no tests currently with input ranges.

[JackStouffer]

Should use the new `` style for D code where ever possible. Same throughout.

[JackStouffer]

Also, I forgot to mention that this should also have unit tests with @nogc @safe nothrow pure.

[wilzbach]

This also needs tests using the dummy ranges, as you have no tests currently with input ranges.
Also, I forgot to mention that this should also have unit tests with @nogc @safe nothrow pure.

Thanks a lot for your review @JackStouffer!
All comments were addressed :)

[PetarKirov]

Nitpick - how about:

alias Element = ElementType!Range;
alias MapType = Unqual!(typeof(mapFun(Element.init)));

Unqual!Element extremeElement = r.front;
MapType extremeElementMapped = mapFun(extremeElement);

(Evaluate r[0] once and make the code a bit more DRY).

[ntrel]

LGTM besides minor comments.

[wilzbach]

LGTM besides minor comments.

Thanks a lot for reviewing this @ntrel - I just addressed your comments.
The only open question so far is whether the specialization for a RandomAccessRange is useful in terms of speed.

[nordlow]

Is having an assert do the emptyness-checking robust enough? Is this the Phobos-way? Alternatively an enforce or explicit throw could be used instead. Or should we rely on the .front-call in these cases? If we rely on .front throwing why do we need the assert-checking for emptyness?

Further, I would like an optional second parameter, seed, (like is used in std.algorithm.iteration.reduce) that elides this throw-logic for flexibility, "potentially" making things nothrow. That is, in the case when range parameter is empty, function returns seed.

Also, it could be motivated to add links in the docs to

• http://en.cppreference.com/w/cpp/algorithm/min_element
• http://en.cppreference.com/w/cpp/algorithm/max_element

I've seen STL-references to algorithms elsewhere in the Phobos docs.

BTW: Thanks for adding these!

[DmitryOlshansky]

Is having an assert do the emptyness-checking robust enough? Is this the Phobos-way?

AFAIK asserting with ranges is the style.

[9il]

AFAIK asserting with ranges is the style.

We need to fix std.numeric thought

[nordlow]

@9il What do you mean by: fixing std.numeric?

[9il]

@9il What do you mean by: fixing std.numeric?

It uses enforce instead of assert. It should be changes to asserts

[9il]

Wait a minute please

[9il]

assert([2, 1, 4, 3].enumerate.reduce!((a, b) => a.value > b.value ? a : b) == tuple(2, 4));

What wrong with this code?

[9il]

If we are so lazy to write lambdas then just allow min/max to accept map function instead ;-)

[wilzbach]

What wrong with this code?

It evaluates the map function twice ;-)

If we are so lazy to write lambdas then just allow min/max to accept map function instead ;-)

Yes I would love to see min working for ranges! See my comment at #4019

As you mentioned a lot of languages (Java, C#, Python, more at Rosetta) allow min to be used with one argument and as still being a newcomer to D I feel that many people like me will just expect [1,2,3].min to work.

[9il]

Another option is add combined mapReduce to the iteration module.

[9il]

It evaluates the map function twice ;-)

So we need combined mapReduce

[9il]

But please do not rush to write it right now. We need to think how this can be generalized for multidimensional Slice

[wilzbach]

btw just a quick update - @9il and I tested for a couple of array types with random access & -release -O3 -boundscheck=off.
-> the variant with RandomAccess is faster (source) (even for ndslice).

[DmitryOlshansky]

@9il is this good to go?

[wilzbach]

@DmitryOlshansky @9il what is your opinion on renaming this to min ? The other min expects two or more parameters and @GassaFM's comment about tuple expansion with a single array in the tuple is seldom and then still understandable.

[9il]

OK, we can move forward. Looks like ndslice will have its own functional

[9il]

@wilzbach min may cause bad behavior with templated code. I prefer current name

[DmitryOlshansky]

Same here - minElement is established name, and single ard min is problematic in generic code

[DmitryOlshansky]

Auto-merge toggled on

[9il]

Hmm... how can I use this new functions with http://dlang.org/phobos/std_math.html#.cmp ?

[wilzbach]

how can I use this new functions with http://dlang.org/phobos/std_math.html#.cmp ?

For reference: see #4257 where I propose to make extremum public ;-)

[nordlow]

Thanks!

[andralex]

Why are minElement/maxElement not in the documentation at https://dlang.org/phobos/std_algorithm_searching.html?

[wilzbach]

Why are minElement/maxElement not in the documentation at https://dlang.org/phobos/std_algorithm_searching.html?

They are part of the 2.072 release (it's quite big due to the long accumulation period - April to September). Hopefully it can be released soon, but with all the regressions popping up it doesn't look good :/

[andralex]

Thanks for the info.