Issue 13595: Extend groupBy to handle non-equivalence relations.

[quickfur]

Update DDocs.

Add unittest for 13595.

Supporting non-equivalence relations (treated as a predicate between adjacent elements) allows for more interesting predicates, like maximum adjacent difference (added as ddoc example), word boundary testing, etc.. This comes at a performance cost, though, so the isEquivRelation parameter was added to fallback to the original implementation when the user is sure their predicate is an equivalence relation.

[bearophile]

Beware of "Boolean Blindness" (http://existentialtype.wordpress.com/2011/03/15/boolean-blindness/ ).

[quickfur]

I thought about that, but recently Andrei seems ill-disposed towards the use of Flags for this purpose. It's trivial to add, though.

[quickfur]

On second thoughts, using an enum for it (like the rest of std.algorithm) seems to be widespread, so I'll settle with that for now. It does make the code more readable, for sure.

[yebblies]

Why not a second name? eg groupByRelation? Then you can avoid the downsides of both a raw boolean and a single-use enum. They'd both forward to the same impl function anyway. Also, maybe normal groupBy should check that the first element compares equal to itself.

[quickfur]

Hmm. That's definitely a possibility, but I don't know if groupByRelation is a good name. The distinction is more in whether the predicate is an equivalence relation or not; the current default is to assume it's not. Sadly, I can't think of a better name to use. Any other ideas?

[quickfur]

Rebased.

[mihails-strasuns]

I like the changes, it is much less confusing for newcomers that way. No idea if groupByRelation is proper name though. @bearophile ?

[quickfur]

Hmm. What about groupByEquivRelation? The idea being that by default, we assume nothing about the predicate, so the nice, short name groupBy handles that case, but if the user knows specifically that something is an equivalence relation, then they can get a slight performance boost by using groupByEquivRelation instead. What do you think?

[mihails-strasuns]

Yeah that makes sense - fits D principle "safe and powerful by default, extra performance if asked for explicitly". Again, won't judge naming choice :P

[MetaLang]

groupByEquivRelation seems far too long to me, in the same realm as binaryReverseArgs, and so nobody will want to use it.

[bearophile]

I like to write, but I am not native English, so my choice of very short names is sometimes not the best.

I think groupByEquivRelation is a bad name. I agree that "binaryReverseArgs" is a too much long name and I am discouraged to use such function just because of its name, it should be renamed "flipTwo" or something similar.

I think that using an enum or Flag is better for this specific case because inventing a fitting name is not easy and it risks becoming a too much specialized function. As usual you need to balance different desires and constraints.

[mihails-strasuns]

Keeping groupBy with flag but making default behavior to not require equivalence is also a valid option.

[mihails-strasuns]

(misclick)

[quickfur]

So everyone agrees that the previous version (two overloads of groupBy with compile-time flag) is better? Should I revert this last commit then?

[mihails-strasuns]

Well, unless some mathematician comes and proposes short and clear name for groupByEquivRelation :P

[quickfur]

What about just groupbyEquiv? With Equiv being short for "equivalence", and an equivalence being shorthand of "equivalence relation". Or is that too much of a stretch? :D

[quickfur]

ping

[mihails-strasuns]

With no new opinions I'd call for going back to version with flag (and using slower version that does not require equivalence as default)

[quickfur]

Alright, reverted back to the version with the flag. Should Flag be used, or is the enum OK for now?

[bearophile]

In my code I'll use this function only rarely. What I usually look for is a hashGroup (that returns an associative array).

[mihails-strasuns]

Should Flag be used, or is the enum OK for now?

I have no preferences on this topic so whatever suits you.

@bearophile I think it should be possible to define .array overload that turns returned range in such associative array. What would be the key though?

[quickfur]

Recently there were some PR's that changed single-use enums into aliases of Flag, like:

alias KeepTerminator = Flag!"keepTerminator";

I think it's a good idea, even though std.algorithm doesn't currently use it. It also lets you use the Yes and No templates -- of questionable value, but at least Phobos should dogfood its own utilities, otherwise what's the use, right?

[quickfur]

Rebased, use Flag instead of single-use enum.

[quickfur]

Renamed.

[mihails-strasuns]

LGTM

[quickfur]

Thanks!

[bearophile]

@Dicebot, the semantics is different so we'll probably need a different function (like hashGroup or something similar).

[DmitryOlshansky]

Auto-merge toggled on

[DmitryOlshansky]

Fine by me

[bearophile]

@quickfur I've found a problem, but I am not sure if the cause is groupBy. I have put the code in Bugzilla, in issue 13595.

[andralex]

I think we need to revert this until https://issues.dlang.org/show_bug.cgi?id=13936 reaches a conclusion.

[quickfur]

Then you'll have to take out #1453 as well, since that's the original implementation. This one is just a fixup.