Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CODE: Added foldr family to library(apply), incl. test cases #727

Closed
wants to merge 3 commits into from
Closed

CODE: Added foldr family to library(apply), incl. test cases #727

wants to merge 3 commits into from

Conversation

dtonhofer
Copy link
Sponsor Contributor

A slight extension of library(apply)

  1. Reviewed comments to make them more complete, especially to newcomers
  2. Added "foldr" family. This is not a big win, but pleasing for symmetry purposes. If we have foldl, why not!
  3. Added units tests for foldl family and foldr family.

@JanWielemaker
Copy link
Member

After recalling some discussion on foldr from the distant past, I did some re-evaluation:

fl(N, Sum) :-
    numlist(1, N, L),
    time(foldl(plus, L, 0, Sum)).

fr(N, Sum) :-
    numlist(1, N, L),
    time(foldr(plus, L, 0, Sum)).

frl(N, Sum) :-
    numlist(1, N, L),
    time((reverse(L, L1),
          foldl(plus, L1, 0, Sum))).

Doing this with a list of 1 000 000 elements, we get times 0.128/0.389/0.281 for the three tests. Using a 10M list foldr runs out of (default) stack, while the other two nicely finish. This suggests to me that it might be a better idea to implement foldr based on reverse+foldl and IMO even better to not implement it at all and instead add a note to the docs for foldl that foldr is simply reverse+foldl.

As a small side note, please use ``` code blocks instead of == for new comments (and typically I replace the == when I edit a comment). `==` comes from older practices in Wiki/Markdown country.

@dtonhofer
Copy link
Sponsor Contributor Author

Yes ... it would be of interest to the student mainly ...

This suggests to me that it might be a better idea to implement foldr based on reverse+foldl and IMO even better to not implement it at all and instead add a note to the docs for foldl that foldr is simply reverse+foldl.

Actually I put a note in there, to get the Prolog student's attention:

As expected with fold-right, the implementation is not subject to tail-call optimization and using foldr is expensive in stack usage

We could just implement foldr/N by reversing the (up to 4) lists passed to foldr/N first, and then apply foldl/N. Done. This would even work in case of open lists.

Plus it's still faster on 4 lists (but I don't have tcmalloc, so maybe...):

fl4(N, Sum) :-
    numlist(1, N, L1),
    numlist(1, N, L2),
    numlist(1, N, L3),
    numlist(1, N, L4),
    time(foldl([A0,A1,A2,A3,FL,TR]>>(TR is A0+A1+A2+A3+FL), L1, L2, L3, L4, 0, Sum)).
       
fr4(N, Sum) :-
    numlist(1, N, L1),
    numlist(1, N, L2),
    numlist(1, N, L3),
    numlist(1, N, L4),
    time(foldr([A0,A1,A2,A3,FR,TL]>>(TL is A0+A1+A2+A3+FR), L1, L2, L3, L4, 0, Sum)).

frl4(N, Sum) :-
    numlist(1, N, L1),
    numlist(1, N, L2),
    numlist(1, N, L3),
    numlist(1, N, L4),
    time(
      (reverse(L1, RL1),
       reverse(L2, RL2),
       reverse(L3, RL3),
       reverse(L4, RL4),
       foldl([A0,A1,A2,A3,FL,TR]>>(TR is A0+A1+A2+A3+FL), RL1, RL2, RL3, RL4, 0, Sum))).

And so:

?- fl4(1000000,Sum).
% 17,041,582 inferences, 5.639 CPU in 5.661 seconds (100% CPU, 3022016 Lips)
Sum = 2000002000000.

?- fr4(1000000,Sum).
% 17,000,101 inferences, 11.461 CPU in 11.527 seconds (99% CPU, 1483288 Lips)
Sum = 2000002000000.

?- frl4(1000000,Sum).
% 21,000,110 inferences, 6.790 CPU in 6.820 seconds (100% CPU, 3092720 Lips)
Sum = 2000002000000.

@dtonhofer
Copy link
Sponsor Contributor Author

Plus, having foldr/N there is a psychological anchor: It's there! Nobody needs to ask about it anymore! Not efficient? We know. It's in its nature.

@JanWielemaker
Copy link
Member

Plus, having foldr/N there is a psychological anchor: It's there! Nobody needs to ask about it anymore! Not efficient? We know. It's in its nature.

This is not really my line of thinking. SWI-Prolog is not a student project. Odd as it may seem, students have never been an explicit target audience. SWI-Prolog is driven by making Prolog work for real world programming. In my view, foldr has no place here, except for a note in foldl on why it does not exist and how to get the same result.

In my view, adding predicates comes at a price: more code, more documentation, more choices, more opportunities for bugs, etc. It is only worth while if there is no easy alternative and it needs to be there for enough applications.

@dtonhofer
Copy link
Sponsor Contributor Author

Thanks Jab,

SWI-Prolog is not a student project. Odd as it may seem, students have never been an explicit target audience.

I'm not suggesting that. In school we used SICStus. But you will have "students" from industry (although it is difficult to get industry interested in anything that is not being hyped in trade mags this month).

Talking of trying to do it via reverse/2 ... I tried and of course it works for proper lists. But once you use open lists, you immediately run into an infinite failure-driven loop when foldl fails in reverse(X,Y),foldl(G,Y,S,V). Once more than one list is involved, you cannot chain the reverse/2 calls anymore (because a redo will just increase the nearest open list), you need to write dedicated "reverse n open lists together" predicates. Ugly.

Bottom line: It's no just reverse + foldl.

I will remove the foldr code but leave in the tests for the foldl. Okay with that?

@JanWielemaker
Copy link
Member

Thanks. Yes, I'm always happy with more tests. Note that the mode of foldl is +List, so formally the behavior on open lists is undefined. As with so many similar predicates there is no check.

As a side node, please keep documentation in source code to the level of a reference manual, i.e., a concise description with a short example when it has added value.

@dtonhofer
Copy link
Sponsor Contributor Author

Note that the mode of foldl is +List, so formally the behavior on open lists is undefined.

I will ask a question about this on discourse, if you will.

As a side node, please keep documentation in source code to the level of a reference manual, i.e., a concise description with a short example when it has added value.

A difficult judgement call! 🤔

P.S.

I have finally found the solution for mono-list foldr that behaves well with open lists. Evident in retrospect:

foldr(Goal,List,Start,Final) :-
   reverse(List,Lrev),
   (foldl(Goal,Lrev,Start,Final)
    ->
    true
    ;
    (!,fail)).

@JanWielemaker
Copy link
Member

A difficult judgement call! thinking

I know. The whole enterprise of evolving a system like SWI-Prolog and its documentation is a continuous trade off. It would be nice if it was possible to write down the rules, but I fear that is hard. Nevertheless, I think it is worth to try and maintain a good balance.

One of the bigger challenges lies at the moment in documentation. That first requires a different infrastructure. We need to deal with formal properties (modes, types, determinism, errors, side-effects, purity, etc.), a summary line, an informal description, examples, tests and tutorials that deal with tasks and puts the related predicates into perspective.

Possibly we need a wiki with appropriate templates? Given the right structure we can probably populate that automatically from all info that is currently there.

Possibly we should discuss such so on Discourse.

@dtonhofer
Copy link
Sponsor Contributor Author

Looks better now. I have just left foldr implementation in there in commented form, in case someone is curious.

@JanWielemaker
Copy link
Member

Ok. Edited, squashed and merged. I've removed foldr. I do not see the point of leaving long commented code in sources. Less is better :) I've also shortened the comment for foldl. The remarks of using library(yall) should be with library yall, or better in some tutorial section as this applies for all meta-calling. A tutorial on yall is surely useful considering all the confusion (not in yall.pl, please). I also deleted the remark on modes. If this is to be supported the mode should change to ?, in which case a short example on why this is useful must be added.

Thanks.

@dtonhofer
Copy link
Sponsor Contributor Author

I reckon this can be closed. Thanks.

@dtonhofer dtonhofer closed this Dec 11, 2020
@JanWielemaker
Copy link
Member

This pull request has been mentioned on SWI-Prolog. There might be relevant details there:

https://swi-prolog.discourse.group/t/add-to-library-apply-any-2-or-include-2-foldl-helper/5971/2

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.

None yet

2 participants