Multiple argument lambda functions #490

ChrisJefferson · 2016-01-18T20:01:47Z

This is a first attempt at multi-argument lambda functions. The patch looks very big, but most of it is rearranging code, to avoid lots of duplication. The main issue I see with this is the notation. I know previous discussions have been had on this before. My only comment is I tried implementing (x,y) -> x + y, and I don't think it's (reasonably) possible, as (x,y) looks too much like a permutation.

The easiest way to demonstrate is with code:

gap> Print({x,y} -> x + y);
function ( x, y )
    return x + y;
end
gap> f := {x,y} -> x + y;
function( x, y ) ... end
gap> f(1,2);
3
gap> f := {x,y...} -> [x,y];
function( x, y... ) ... end
gap> f(1,2,3,4);
[ 1, [ 2, 3, 4 ] ]

fingolfin · 2016-01-28T12:11:59Z

This would be nice to have, IMHO. Just like @ChrisJefferson, the only thing I am mildly concerned about is whether using { for this might block future syntax extensions -- I explained this previously on issue #37 -- specifically, I was wondering whether maybe in the future we might want to use it for a nice shorthand for entering Python-style dictionaries, as in this made-up example var := {"x" := 1, "y" := "abc" }; which would assign to var a new dictionary mapping the two strings "x" and "y" to the integer 1 resp. the string "abc".

But my hope is that those two uses would not clash (except perhaps from an aesthetical point of view).

ChrisJefferson · 2016-01-28T12:16:11Z

This shouldn't clash, we can handle it in the same way we already have to handle that x might be the start of x := 1 or x -> x, in the same way {x might be the start of {x,y} -> x+y or {x := 1}. In both cases we know after the second symbol (in the case of records it is a comma, or a :=).

Of course, if the saving of '{ }' over 'rec( )' is enough is another question!

fingolfin · 2016-01-28T12:23:04Z

This is not about a replacement syntax for rec(), but rather about a hypothetical syntax for a completely new data type (possiblye provided by the datastructures package). I.e a proper dictionary, instead of the "fake" one provided by GAP records.

ChrisJefferson · 2016-01-28T12:29:34Z

Ah, that sounds very reasonable. I'll do the parsing once someone else does the datastructure :) (but let's discuss that somewhere else, at a later date).

rbehrends · 2016-01-28T15:37:39Z

The problem with parsing a hypothetical (x, y) -> x + y syntax is of course that code generation/evaluation is so tightly interleaved with parsing. I'm wondering if making these parts into separate phases would be worth it (this would also mean that evaluating code as you parse it would no longer be possible, so there are downsides). Not just because of a nicer syntax for anonymous functions, but because in the long term we might really need a more flexible internal representation (for things such as optimization or native code generation). I realize that this would be a fairly big undertaking, of course.

fingolfin · 2016-01-28T16:10:57Z

@rbehrends Not sure if this is what you mean, but my understanding (as was explained on issue #37) is that the only issue with (x, y) -> x + y is that we have a special case for parsing permutations, correct? And this is why @ChrisJefferson here implemented a different syntax...

Anyway, thinking about that again, I am actually somewhat confused as to why that really is a problem: Basically, after the first comma, we know whether we are looking at e.g. (1234, ... (can only be a permutation, cannot be a lambda), or at at e.g. (1234x, ... (cannot be a permutation, could be the start of a lambda expression). Can't we use that to break the ambiguity? Maybe this is not possible with the current parser/scanner setup, I didn't look at it, just thought about how I would parse this naively.

fingolfin · 2016-01-28T16:12:27Z

Oh wait, now I see, you can actually do this in GAP:

gap> i:=1;
1
gap> (i,2,3);
(1,2,3)

Wow didn't realize this was possible. OK, that kills that idea... (Though, is anybody using that feature? Is the library using this anywhere?)

olexandr-konovalov · 2016-01-28T16:49:04Z

@fingolfin wow - never knew that this is possible.

ChrisJefferson · 2016-01-28T17:24:56Z

I've used the following code, to get all permutations which permute two elements, so there is at least one use :) It is hard to search the library, and packages, for uses, as the notation looks too much like a function call.

Union(List([1..100], x -> List([x+1..100], y -> (x,y) )));

rbehrends · 2016-01-28T20:02:32Z

On second thought, it seems to be possible to do this without completely rewriting the parser, borrowing ideas from attribute grammars (attribute grammars can attach semantic information to non-terminals to allow for context-sensitive parsing). While parsing a permutation, you'd track a "maybe a function" flag that remains true as long as the permutation items are identifiers). If that flag becomes false or if the closing parenthesis is not followed by ->, you retroactively turn the list of identifiers into a permutation. This should be doable with finite lookahead.

ChrisJefferson · 2016-01-29T11:46:51Z

While we could do that, there would be issues, as the GAP error reporting assumes we find errors immediately. To see what I mean, consider the following code (notice the placements of the ^). If we waited until the closing ) before deciding that we had a permutation, the ^ would all line up with the ).

gap> x -> (i,j,k)
Syntax warning: Unbound global variable
x -> (i,j,k)
       ^
Syntax warning: Unbound global variable
x -> (i,j,k)
         ^
Syntax warning: Unbound global variable
x -> (i,j,k)
           ^

frankluebeck · 2016-02-01T15:53:16Z

Hm, the sytax

{x,y} -> x + y

doesn't look very GAP like. Three more ideas:
Use a keyword instead of {}s:

function x, y -> x + y

or, if a double use of function is bad for some reason, a new one (python like):

lambda x, y -> x + y

or, if braces are ok, use them around the whole function definition:

{ x, y -> x + y }

Unfortunately, none of these specializes naturally to the case of one argument which we already have.

markuspf · 2016-10-20T09:23:34Z

Can we somehow get this moving and to a decision?

olexandr-konovalov · 2016-11-18T18:18:56Z

We need to agree about the syntax. I will be happy with

{x,y} -> x + y

considering {x,y} as a special tuple of arguments, but wouldn't mind

lambda x, y -> x + y

or

{ x, y -> x + y }

as well. Unless there will be an obvious agreement, we can used doodle again like we did in #391.

fingolfin · 2016-11-24T15:26:20Z

@frankluebeck I don't understand you remark about this not being "GAP like" - we are talking about a syntax extension here, so of course it's not "GAP like", and I don't find your three alternatives "GAP like" either.

Also note that lambda is used by tons of packages, so we can't just turn it into a keyword. And using function x,y -> x+y (or the "more GAP like" function(x,y) -> x+y kind of defeats the purpose of allow this to be used as a super concise way of writing things like this:

Sort(myData, {x,y} ->  x^2 < y^2);

Personally, I am fine with {x,y} -> x+y. But if people want alternatives, here are two more basd on #37:

& x,y -> x+y;
&(x,y) -> x+y;

ChrisJefferson · 2016-12-19T22:20:13Z

Just one comment on something @frankluebeck said: Indeed none of our suggestions generalise down to the old single-argument version. However, I think that shows the single-argument version was a mistake -- handling it is a really nasty hack in the parser (although, we will obviously never remove the old single argument version, it has been around for far too long).

fingolfin · 2016-12-21T18:22:07Z

So, perhaps we should make a doodle with some of the options, to get an idea who supports what? Anybody willing to set one up? :)

olexandr-konovalov · 2016-12-21T22:19:37Z

Why not - it worked in #391. I've created the poll and added all options in the order in which they have appeared in this thread (if you want to suggest more options, please let me know ASAP):

(x,y) -> x + y
{x,y} -> x + y
function x, y -> x + y
lambda x, y -> x + y
{ x, y -> x + y }
& x,y -> x + y
&(x,y) -> x + y

Please try to make your choice at http://doodle.com/poll/8tty9qg68tz9stc7 within the next several days, to ensure that we make some progress. For each option, you need to specify one of the three possibilities:

Yes : for your favourite options(s)
(Yes) : for "if-need-be" option(s) which you would accept
No : for options(s) which you categorically reject.

It's fine to select "Yes" more than once. Also it's fine to change your mind later and edit your votes. Non-members of the gap-system and gap-packages GitHub organisations may vote too, especially if you have a strong opinion, but please note this is not a binding vote: we only want to have an overview what's the preferred syntax, and hopefully it will be easier for us to make a choice.

olexandr-konovalov · 2016-12-21T22:49:26Z

Interesting to think how such notation will look if nested. This hypothetical code takes two arguments and returns a function of another two arguments:

{ x,y -> {z,t -> (x+y)*(z+t) } }

which is good because of matching delimiters.

In another notation, it would look like

{x,y} -> {z,t} -> (x+y)*(z+t)

which looks neat resembling functional programming style, but likely require brackets, what makes it less neat:

{x,y} -> ( {z,t} -> (x+y)*(z+t) )

hungaborhorvath · 2016-12-21T22:59:10Z

Is [x,y] -> x+y completely out of the question? I find this more GAP-like that any of the other suggestions. Or would it be too hard to make happen?

olexandr-konovalov · 2016-12-21T23:02:55Z

@hungaborhorvath [x,y] is a list - same considerations as when (x,y) is a permutation above (that option occurred in the poll only because it was expressed in this thread). Do you want to nominate &[x,y] -> x+y?

fingolfin · 2016-12-22T00:05:46Z

@alex-konovalov why would it require bracketing? I see no other way of interpreting that expression - perhaps I am overlooking something?

olexandr-konovalov · 2016-12-22T00:12:29Z

@fingolfin to help the parser? that was just an assumption, if it could be implemented without brackets - that's even better. Indeed, we don't need brackets to read e.g.

{x,y} -> {z,t} -> {u,v} ->(x+y)*(z+t)*(u/v)

ChrisJefferson · 2016-12-22T11:20:35Z

The problem with [x,y] is the same as (x,y), we don't know it's the beginning of a function parsing until the end when we find the arrow.

That means we need to parse the expression preparing for either case (either introducing new quantifiers, or finding expressions), and finalising at the end. That is possible, but it gets more horrible when we add various natural extensions, like handling 'readonly' arguments in HPC-GAP, or doing (x,y...) -> f(x,y). On the other hand, using something like {} or & means we know we are argument parsing, and there is no possible confusion.

fingolfin · 2017-01-04T14:22:11Z

It seems {x,y} -> x+y is a clear winner in that informal poll...

@ChrisJefferson would you mind updating this PR, rebased on latest master, and adjusted to that syntax, so that we can try it out?

ChrisJefferson · 2017-01-06T10:02:49Z

Updated, try it out and see if there are any issues or tweaks.

We can now write zero-argument lambdas in two ways, -> 2 and {} -> 2, and one-argument lambdas as x -> x or {x} -> x. While I don't like the redundancies, I don't want to delete the old format, and like the consistency of always allowing the {..} notation.

There are a number of possible extensions we could have on top of this. I'm happy to look into implementing them, but I would ask they get put into new issues unless they would change the user-visible changes in this pull request.

codecov-io · 2017-01-06T10:17:57Z

Current coverage is 49.73% (diff: 92.38%)

Merging #490 into master will increase coverage by 0.07%

@@             master       #490   diff @@
==========================================
  Files           424        424          
  Lines        223275     223469   +194   
  Methods        3429       3432     +3   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         110880     111136   +256   
+ Misses       112395     112333    -62   
  Partials          0          0

Powered by Codecov. Last update fd9d4e3...d235546

fingolfin · 2017-01-06T13:36:06Z

@ChrisJefferson Thanks for the update! I'll try to review it.

Regarding what you wrote: Of course one-argument lambdas in the old syntax must still be supported, there is far too much code relying on it. I am less certain about zero-argument lambdas; those are mostly specific to HPC-GAP, and not much if anything outside of that uses them. So we could still abolish that particular syntax in the future. But of course this PR is not where that would happen.

fingolfin

This looks pretty solid to me. I only have nitpicky formatting and coverage comments, and an off-topic question.

fingolfin · 2017-01-06T14:25:48Z

src/read.c

+            case S_READWRITE:
+            if (!is_atomic) {
+                SyntaxError("'readwrite' argument of non-atomic function");
+                GetSymbol();


Note that tests do not cover this code block -- I guess we'd have to add another test to tst/testinstall/atomic_basic.tst where a readwrite argument occurse in second or later position.

You can of course rightfull argue that this code was not covered before, and you just moved it. Also fine, I just wanted to point it out ;-)

I added tests for this, and also for lambda (at the moment they are never valid for lambda, as you can't put the initial atomic. We could invent a notation for that, if we ever want one).

fingolfin · 2017-01-06T14:27:34Z

src/read.c

+*F  ReadFuncArgList( <follow>, <is_atomic>, <is_block>, <symbol>, <symbolstr> )
+**  . . . . . . . . . .  read a function argument list.
+**
+**  'ReadFuncArgList' reads the argument list of a function. In case of an error


This line is too long 8-)

fingolfin · 2017-01-06T14:28:05Z

src/read.c

@@ -1469,28 +1526,21 @@ void ReadFuncExpr (

 /****************************************************************************
 **
-*F  ReadFuncExpr1(<follow>) . . . . . . . . . . .  read a function expression
+*F  ReadFuncExprBody(<follow>) . . . . . . .   read the body function expression


This line is too long (remove some of the . ).

fingolfin · 2017-01-06T14:29:39Z

src/read.c

@@ -1527,6 +1579,62 @@ void ReadFuncExpr1 (

 /****************************************************************************
 **
+*F  ReadFuncExprLong(<follow>) . . . . . read a multi-arg function expression
+**
+**  'ReadFuncExprLong' reads  an abbreviated  function literal expression.  In


Again a comment line is a bit too long. Just remove some of the double spaces (I think trying to justify format our comments is a silly waste of time anyway).

Of course you may argue that limiting line lengths is silly, too ;-). But then I'd get rid of the /*****..., too, as it looks weird to have that and then go wider right after it...

fingolfin · 2017-01-06T14:31:08Z

src/read.c

+    TypSymbolSet        follow )
+{
+    volatile Obj        nams;           /* list of local variables names   */
+    volatile Obj        name;           /* one local variable name         */


Off-topic question: why are these local variables here and in the other functions volatile (I know that you just copied it, but perhaps you know)? Is it for HPC-GAP? But why exactly?

I have no idea. I did just copy it, but I'm positive it can't be doing anything useful.

They are however everywhere, and very old (they come from pre-history, not HPC-GAP), so I'll leave them for now and clean them up later.

Might relate to break loops and the possibility of longjumping in or out of bits of parser. Not sure how,

I don't see how that would be related either... We could just try it by making a PR which removes all volatile, and see if anything breaks :) See PR #1084

fingolfin · 2017-01-06T14:33:01Z

src/read.c

+        nams = NEW_PLIST( T_PLIST, narg );
+        SET_LEN_PLIST( nams, narg );
+        TLS(CountNams) += 1;
+        ASS_LIST( TLS(StackNams), TLS(CountNams), nams );


This code is not covered by tests. I assume it is for do/od blocks with locals? Do we have tests for do/od blocks already?

In GAP we disabled the parsing for do od blocks, so I'm fairly sure this can never get called, but obviously haven't removed all the associated code.

Thanks. (I think it would be nice to have do/od blocks in non-HPC GAP, too, but that's clearly beyond the scope of this PR)

fingolfin · 2017-01-06T14:35:54Z

Great work. I think the next steps would involve testing this, and documenting it. Ideally, I'd like to see documentation before merging this (as once it's merged, it's there, and we might end up delaying to write documentation for far too long, ending up with a GAP release with this feature undocumented).

Add support for functions of the form {x,y} -> result. This is actually a very small change, most of the code churn is seperating out function argument parsing so it can be used in both traditional functions and lambda functions.

fingolfin · 2017-01-06T17:23:01Z

doc/ref/language.xml

-variable number of arguments this way.
+<A>arg-list</A> is a (possibly empty) argument list. Any arguments list
+which would be valid for a normal GAP function is also valid here (including
+variadic arguments).


ohh, variadic, nice :)

fingolfin · 2017-01-06T17:24:55Z

doc/ref/language.xml

@@ -1822,23 +1822,55 @@ Also see Chapter&nbsp;<Ref Chap="Functions"/>.
 <P/>
 <Index Subkey="definition by arrow notation">functions</Index>
 <Index>arrow notation for functions</Index>
-<C><A>arg-ident</A> -> <A>expr</A></C>
+<C>{ <A>arg-list</A> } -> <A>expr</A></C>


So the old syntax is not documented anymore, is it? But there are still examples for it -- and I think we should still document it. Perhaps like this?
"If the argument list consists of a precisely a single argument, you can also omit the braces."

Carry on a little further (although, it might be a little hidden at the end).

fingolfin · 2017-01-06T18:39:26Z

doc/ref/language.xml

+]]></Example>
+<P/>
+The <C>{</C> and <C>}</C> may be omitted for
+functions with one and zero arguments:


Ahh, here. I overlooked this, sorry.

Hmm, I am still not sure I am happy to document the zero argument variant. I know it's there for HPC-GAP, but I never liked it, and I am not sure we agreed to make it official. I really prefer {}->FOOBAR over ->FOOBAR.

I'll not mind terribly if it stays in, though -- but then we should make it clear that this PR also turns this so far inofficial addition into an official one (i.e. let's not "sneak" this in).

Personally I dislike it, but it's in 4.8, so technically we'd be removing an (undocumented) feature from a stable release. I'm happy to do that, if we decide to do that "officially" too :)

fingolfin · 2017-01-09T18:57:36Z

Perhaps it's time to mention this PR on the public gap mailing list, to give a broader audience a last chance to chime in; and then (assuming there is no really good counter argument), merge it in a week or so?

@ChrisJefferson Would you consider writing that email?

In PR gap-system#490 we were wondering why these volatile keywords are there. Let's find out by removing them, and testing the result...

fingolfin · 2017-01-20T20:01:14Z

I sent a mail on this to the GAP mailing list on Monday, February 16. I set a reminder for myself, and if there are no protests by coming Wednsday, I'll just merge it.

ChrisJefferson · 2017-01-20T22:17:24Z

Thanks, sorry, I missed you message asking me to write the mail. Too much github traffic recently, I need a better way of making sure I read all the messages I should deal with...

ChrisJefferson force-pushed the lambda branch 2 times, most recently from a4b815e to 8b125cf Compare January 18, 2016 20:30

olexandr-konovalov mentioned this pull request Jan 18, 2016

Syntax extension: inline function declaration with two or more parameters #37

Closed

ChrisJefferson force-pushed the lambda branch from 8b125cf to 0cfee60 Compare January 18, 2016 20:46

olexandr-konovalov added the gapsagedays2016 Issues and PRs that arose at https://www.gapdays.de/gap-sage-days2016 label Jan 18, 2016

markuspf added this to the GAP 4.9.0 milestone Mar 7, 2016

ChrisJefferson force-pushed the lambda branch from 0cfee60 to d1f2050 Compare January 6, 2017 09:50

This was referenced Jan 6, 2017

Strange behaviour of ~ #1049

Closed

Tilde fix #1050

Closed

fingolfin reviewed Jan 6, 2017

View reviewed changes

ChrisJefferson force-pushed the lambda branch from 82131be to a37d10f Compare January 6, 2017 17:20

ChrisJefferson added 2 commits January 6, 2017 17:24

Add lambda functions.

ba5ca74

Add support for functions of the form {x,y} -> result. This is actually a very small change, most of the code churn is seperating out function argument parsing so it can be used in both traditional functions and lambda functions.

Add documentation for multi-argument lambda functions

e374e5d

ChrisJefferson force-pushed the lambda branch from a37d10f to 0422a6c Compare January 6, 2017 17:24

Add atomic tests

025710a

ChrisJefferson force-pushed the lambda branch from 0422a6c to 025710a Compare January 6, 2017 17:24

fingolfin reviewed Jan 6, 2017

View reviewed changes

Add tests to catch repeated argument and local names

d235546

fingolfin reviewed Jan 6, 2017

View reviewed changes

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature labels Jan 6, 2017

fingolfin added a commit to fingolfin/gap that referenced this pull request Jan 17, 2017

experiment: remove volatile from read.c and stream.c

fdb5250

In PR gap-system#490 we were wondering why these volatile keywords are there. Let's find out by removing them, and testing the result...

fingolfin mentioned this pull request Jan 17, 2017

experiment: remove volatile from read.c and stream.c #1084

Closed

fingolfin merged commit 8be85de into gap-system:master Jan 25, 2017

fingolfin mentioned this pull request Jan 25, 2017

Get rid of undocumented syntax ->1 for argumentless lambdas #1102

Closed

ChrisJefferson deleted the lambda branch February 5, 2017 10:36

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov changed the title ~~[WIP] Multiple argument lambda functions~~ Multiple argument lambda functions Jan 20, 2018

Multiple argument lambda functions #490

Multiple argument lambda functions #490

Conversation

ChrisJefferson commented Jan 18, 2016

fingolfin commented Jan 28, 2016

ChrisJefferson commented Jan 28, 2016 • edited by fingolfin Loading

fingolfin commented Jan 28, 2016

ChrisJefferson commented Jan 28, 2016

rbehrends commented Jan 28, 2016

fingolfin commented Jan 28, 2016

fingolfin commented Jan 28, 2016

olexandr-konovalov commented Jan 28, 2016

ChrisJefferson commented Jan 28, 2016

rbehrends commented Jan 28, 2016

ChrisJefferson commented Jan 29, 2016

frankluebeck commented Feb 1, 2016

markuspf commented Oct 20, 2016

olexandr-konovalov commented Nov 18, 2016

fingolfin commented Nov 24, 2016

ChrisJefferson commented Dec 19, 2016

fingolfin commented Dec 21, 2016

olexandr-konovalov commented Dec 21, 2016 • edited Loading

olexandr-konovalov commented Dec 21, 2016

hungaborhorvath commented Dec 21, 2016

olexandr-konovalov commented Dec 21, 2016

fingolfin commented Dec 22, 2016

olexandr-konovalov commented Dec 22, 2016

ChrisJefferson commented Dec 22, 2016

fingolfin commented Jan 4, 2017

ChrisJefferson commented Jan 6, 2017

codecov-io commented Jan 6, 2017 • edited Loading

Current coverage is 49.73% (diff: 92.38%)

fingolfin commented Jan 6, 2017

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 9, 2017

fingolfin commented Jan 20, 2017

ChrisJefferson commented Jan 20, 2017

ChrisJefferson commented Jan 28, 2016 •

edited by fingolfin

Loading

olexandr-konovalov commented Dec 21, 2016 •

edited

Loading

codecov-io commented Jan 6, 2017 •

edited

Loading