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

map and forEach don't appear to work in the command line #665

Closed
mickmccauley opened this Issue May 17, 2016 · 13 comments

Comments

Projects
None yet
3 participants
@mickmccauley

mickmccauley commented May 17, 2016

Hi,

I'm trying to get map or forEach to work in the command line

For example, let's say I have:

a = [1,2,3]
f(x) = x*x

I have tried the following but to no avail.

Any ideas on what the right syntax is or is this a bug?

map(a, f)
SyntaxError: Wrong number of arguments in function f (3 provided, 1 expected)

map(a, f(x))
Error: Undefined symbol x

Or using the syntax suggested in http://mathnotepad.com/docs/functions.html#function=map

map(a, function(value) { return value * value })
SyntaxError: Parenthesis ) expected (char 32)

Thanks,
Mick.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented May 17, 2016

The documentation is quite misleading indeed.

I've looked at the implementation and it seems that you need a function that actually implements all three parameters value, index and matrix. In your example, this would be:

a = [1,2,3]
f(x,y,z) = x * x
map(a,f)

The example in the documentation uses the math.map function directly from JavaScript, so it get's away with not having 3 parameters.

@josdejong: Is this a bug? Should map accept all kinds of functions created with FunctionAssignmentNode?

This should either be fixed or clarified in the documentation.

@mickmccauley

This comment has been minimized.

mickmccauley commented May 17, 2016

Great. Thanks. I'm delighted that it works. I suggest just improving the documentation for the moment. Can I close this or should I leave it open until a decision is made on fix code or doc?

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented May 17, 2016

Leave it open for now until this has been discussed with Jos.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 17, 2016

hm, functions created via the expression parser indeed validate the number of inputs. This may do more harm than good, like when using it via map which passes three arguments to the callbak function.

Makes sense to me to change this behavior and simply remove this check.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 18, 2016

This is an interesting issue. I've been thinking about this in the past too: this issue occurs too when you pass any typed function to map and forEach, for example:

map([4,9,16], sqrt)   
    // TypeError: Too many arguments in function sqrt (expected: 1, actual: 3)

In general it's a good thing that functions throw an error when passing the wrong number or type of arguments. Options are:

  1. Let typed-functions not throw an error when passing too many arguments, and silently ignore them

  2. Accept that it's like this and use wrapper functions to get work done (inconvenient but functional):

    map([4,9,16], f(x, index, array) = sqrt(x))
    
  3. Make functions map and forEach smarter: let them check the passed callback function. When it's a typed-function check whether there is a signature available with either 1, 2, or 3 arguments and adjust the way the callback is invoked to match the available signatures.

I like option 3 most, I think it shouldn't be too complicated to implement this.

Besides this, I still think it's better to remove the check of matching argument count from function definitions in the expression parser: it's only a half solution and if you want to define a typed function you can simply do so using typed({ signature: fn }).

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented May 18, 2016

I also like option 3 the most.

I'm not quite sure if I understand your "besides". You mean checking the number of arguments that are passed to a function that has been defined via the expression parser?

If that's what you mean, I think they should check the number of arguments they are getting passed. Or is there a benefit in not doing so over the trouble that it introduces by not providing good error messages in cases where someone calls it with the incorrect number of arguments.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 18, 2016

Hm that got me thinking again.

What I meant is that currently, when you create a function like math.eval('f(x)=x*x'), the function is checks the number of arguments but cannot check the type. So this is an incomplete solution and it may be better to just remove this.

What you can do is create a typed function like:

math.eval('f = typed({"number": f(x) = x * x})')

On the other hand, we could improve the expression parser to make it easy to create a real typed-function, something like:

math.eval('f(x: number) = x * x')

that would be even nicer

josdejong added a commit that referenced this issue May 18, 2016

josdejong added a commit that referenced this issue May 18, 2016

Fixed #665: functions `map`, `forEach`, and `filter` now invoke callb…
…acks which are a typed-function with the correct number of arguments
@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented May 19, 2016

That sounds like a good idea if it's easy to implement without overly complicating the parser.

But what about supporting multiple types for a parameter?

Something like this?

math.eval('f(x: number,bignumber) = x * x')

Or make it possible to create multiple implementations of a function?

math.eval('f(x: number) = x * x; f(x: bignumber) = x * x')
@josdejong

This comment has been minimized.

Owner

josdejong commented May 22, 2016

hm. this can work but it's based a bit on a side effect of the expression parser "accidentally" having sort of the same syntax as typed-function signatures. Thing is that the parser only knows that it's dealing with a function definition when it encounters the = character, and at that point the left hand side f(x: number) must already be parsed. The left hand side would be parsed first as a function call containing a range from variable x to variable number. It's valid syntax and we could afterwards parse it as a variable name and data type but feels a bit shaky.

The only syntax of typed-function that's would not work right away is variable arguments ...args, but it can be that typed-function is extended in the future with new options (like optional arguments) which syntax might conflict with that of the mathjs parser. On the other hand, we can take care that these two play nice together and it may not be a problem at all.

Multiple types for a parameter would be defined as :

math.eval('f(x: number | BigNumber) = x * x')

and multiple signatures would really require using typed:

math.eval('f = typed({"number": f(x) = x * x, "BigNumber": f(x) = x.times(x)})')

.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented May 22, 2016

Hm, maybe it's better to just leave it as it is and document the possibility of using typed directly with the expression parser instead of creating two ways to solve the same problem.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 22, 2016

Ok I will add this to the docs.

I'm still not sure whether we should keep checking the number of arguments or should remove it. Being able to create a function which doesn't do any checking can be beneficial too (more flexible, better performance). Maybe an all-or-nothing approach is best, where by default there are no checks being done at all unless you create a typed-function.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented May 22, 2016

I think the number of arguments should still be checked. I can't think of a situation where one could create a variadic function via the expression parser (is there?). In that case what good will it do to allow calling it with the wrong number of arguments?

Instead all functions that get a function as argument (like forEach and map) should manually bypass the check.

Does this make sense?

@josdejong

This comment has been minimized.

Owner

josdejong commented May 22, 2016

You are right it's a bit of a hypothetical case I think. Only thing I can come up with is using a function created with the expression parser as callback for JavaScript's native Array.map function for example.

Ok let's close this discussion and leave the check for the number of arguments there. The original issue has been solved by improving map, forEach, and filter, and the docs are updated too.

@FSMaxB FSMaxB closed this May 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment