Skip to content

Conversation

monarchdodra
Copy link
Collaborator

This is a preliminary rough sketch to add a new "fold" to algorithm. It is designed as a replacement to reduce:

  • UFCS-friendly args.
  • More customizable behavior (foldl/foldr for left/right associativity) (seed: fold/fold1 for T.init/front)
  • More seed customization (0, 1 or N seeds)

This is a proof of concept submission. I'd be most interested in reviewing the API/design first. If/when we decide the API is good, I'd be more than happy to nitpick away at the code.

Pinging @bearophile : I'm not sure if this is exactly what haskell does, but I think this design makes sense for D? but can you give me your opinion?

@monarchdodra
Copy link
Collaborator Author

Two relevant bug reports about fold over reduce are:
Issue 8755 - Change the order of reduce arguments
https://d.puremagic.com/issues/show_bug.cgi?id=8755

Issue 10670 - std.algorithm.reduce: no-seed initialization wrong design
https://d.puremagic.com/issues/show_bug.cgi?id=10670

std/algorithm.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"of of"

@Poita
Copy link
Contributor

Poita commented Feb 22, 2014

What is the purpose of foldl1 and foldr1? Why not just do what reduce does an enforce when the range is empty and no seed is provided?

EDIT: to elaborate, I would expect this:

[1, 2, 3].fold!min() == 1; // no default seed used
[1].fold!min() == 1; // same as above, no default seed used
[].fold!min(); // runtime assert
[].fold!min(1) == 1; // use provided seed
[1, 2, 3].fold!min(4) == 4; // use provided seed

std/algorithm.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

respectively (no double L).

@Poita
Copy link
Contributor

Poita commented Feb 22, 2014

Is foldr even necessary? Doesn't foldr!(f)(r) == foldl(binaryReverseArgs!f)(r.retro)?

std/algorithm.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this fail if I try to call foldl1 or foldr1 on an empty range?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to place an assert here, but yes, it will fail. That's the point (and what reduce currently does).

@Poita
Copy link
Contributor

Poita commented Feb 22, 2014

On the naming side of things, I think foldLeft and foldRight would be more in keeping with Phobos naming conventions (we tend to avoid needless abbreviation).

@monarchdodra
Copy link
Collaborator Author

Is foldr even necessary? Doesn't foldr!(f)(r) == foldl(binaryReverseArgs!f)(r.retro)?

I thought of that, but (IMO) that reads awfully complicated. I think it's just as easy to have a named function that does this. It helps keep the interface complete, and is common with haskel's, so consistent cross-language.

@monarchdodra
Copy link
Collaborator Author

On the naming side of things, I think foldLeft and foldRight would be more in keeping with Phobos naming conventions (we tend to avoid needless abbreviation).

Well... arguably foldFront and foldBack maybe? But as I said, the function names come from Haskel. I don't have a strong opinion on it though.

@monarchdodra
Copy link
Collaborator Author

What is the purpose of foldl1 and foldr1? Why not just do what reduce does an enforce when the range is empty and no seed is provided?

The problem isn't empty range, but of knowing whether or the first element should itself be passed through the pred, or used as the first seed. Both scenarios are valid.

auto sumSquares = [2, 3, 4].fold"a + b^^2"(); //    0 + 4 + 9 + 16
                                              //NOT 2 + 9 + 16
auto concat = ["hello", "there", "world"].fold1!"a ~ b"(); //    "hello there world"
                                                           //NOT " hello there world"

@Poita
Copy link
Contributor

Poita commented Feb 22, 2014

Haskell uses foldl and foldr but other languages (e.g. Scala and Scheme) don't abbreviate, so it isn't without precedent. Phobos generally avoids abbreviation, so I would suggest using foldLeft and foldRight.

EDIT: I don't like foldFront and foldBack because the left/right refer to the associativity.

@Poita
Copy link
Contributor

Poita commented Feb 22, 2014

Not sure I follow the argument for the fold*1 versions. If you want the first element to be passed through the pred, you don't provide a seed. Your first example is just bad code, the user should write [2, 3, 4].map!"a^^2".fold!"a+b", or just provide the 0 seed manually.

@monarchdodra
Copy link
Collaborator Author

Your first example is just bad code, the user should write [2, 3, 4].map!"a^^2".fold!"a+b", or just provide the 0 seed manually.

Maybe you are right. For what it's worth, I converting the reduce unittests to use fold, and having the seed default to the first value is much more convenient (as opposed to T.init == nan). Furthermore, since I allowed having "single seed for multiple functions" writing: r.reduce!(fun1, fun2, fun3)(0.0) isn't the end of the world either... So if only for making any sort of migration possible, it could indeed be worth dropping fold?1.

One thing I did notice that I like about the way I wrote the code is having a vararg arguments, as opposed to a tuple arg, eg:

r.reduce!("a + b", "a * b")(tuple(0.0, 1.0)); //Assuming UFCS
vs
r.fold!("a + b", "a * b")(0.0, 1.0); //Neater, IMO

@monarchdodra
Copy link
Collaborator Author

TLDR : @Poita I think I agree that simply defaulting to "seed is front when no seed is passed, and dropping fold1" is better. Especially given the existing behavior of reduce. As for the "trio" of fold, foldl, foldr, I'm unsure.

To be honest, I haven't found any real usecase for foldr. In any case, no use case that couldn't be applied to another algorithm, which doesn't actualy have an r verson anyway.

So I think I'll drop them and just leave it at fold.

I'll leave this open for a day or two, should anybody else have any suggestions, and then I'll close it to pursue work on inplementation.

@Poita
Copy link
Contributor

Poita commented Feb 22, 2014

I suppose the elephant in the room is whether or not @andralex is going to be happy with having both reduce and fold in Phobos...

@monarchdodra
Copy link
Collaborator Author

I suppose the elephant in the room is whether or not @andralex is going to be happy with having both reduce and fold in Phobos...

The issue of reduce and UFCS has been a recurring "problem" that can only be solved with a new named function. It's been a long standing thorn. Also, the idea is that fold is a drop-in replacement for reduce: they aren't meant to co-exist. That, and I think the interface is nicer, which gives the new fold a nice facelift compared to reduce.

@andralex : If you have time to read this, your input would be appreciated.

@bearophile
Copy link

For now I suggest to add just fold, that acts like reduce, with swapped arguments. More folds can be added later if needed.

@monarchdodra
Copy link
Collaborator Author

For now I suggest to add just fold, that acts like reduce, with swapped arguments. More folds can be added later if needed.

yes, that's the idea. I have the rough code ready, it needs some finishing touches.

@quickfur
Copy link
Member

ping
Is this getting anywhere? I'd like to get fold into Phobos within this lifetime, if at all possible. ;-)

@monarchdodra
Copy link
Collaborator Author

I've been busy, but my re-implementation of reduce should mean it becomes easier to add fold (as I imagine it) to phobos now. I'll try to get on it.

@HK47196
Copy link
Contributor

HK47196 commented Aug 31, 2015

@monarchdodra
Copy link
Collaborator Author

ping, status?

Sorry, the current status is that I will not be implementing this, as I am way too busy.

That said, the code for reduce should be ready for use with fold directly, and should be relatively easy to do from a code perspective. Passing the review might be harder, are there were (I remember) points of contention.

@wilzbach
Copy link
Contributor

The issue of reduce and UFCS has been a recurring "problem" that can only be solved with a new named function. It's been a long standing thorn.

I know that pings are annoying, but I ran into this issue on my first day of using D.
If I understood it correctly the PR just needs (1) to be rewritten to only fold, (2) to be completed by more tests and (3) maybe add a deprecating warning for reduce.

If that's it, I would be happy to help out and fix the missing bits ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants