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

more support for operation nodes with multiple args #739

Closed
evykassirer opened this Issue Nov 6, 2016 · 13 comments

Comments

Projects
None yet
4 participants
@evykassirer

evykassirer commented Nov 6, 2016

If I create an operation node with list of 3 arguments

  • toString() doesn't use the operator symbol anymore

e.g.
var node1 = new math.expression.node.ConstantNode(2);
var node2 = new math.expression.node.ConstantNode(3);
var node3 = new math.expression.node.ConstantNode(4);
var sum = new math.expression.node.OperatorNode('+', 'add', [node1, node2, node3]);

sum.toString() --> "add(2, 3, 4)"

I propose that this print "2 + 3 + 4" instead. I think toTex() has the same issue

  • eval() doesn't evaluate the sum anymore

sum.eval() --> VM221:48 Uncaught TypeError: Too many arguments in function add (expected: 2, actual: 3)

I propose that since Operator nodes take a list of arguments and not exactly two arguments, they should be able to support eval() for any number of arguments in the arg list

@evykassirer evykassirer changed the title from toString and toTex supporting operation nodes with multiple args to operation nodes with multiple args Nov 6, 2016

@evykassirer evykassirer changed the title from operation nodes with multiple args to more support for operation nodes with multiple args Nov 6, 2016

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 6, 2016

What is the usecase for this?

@evykassirer

This comment has been minimized.

evykassirer commented Nov 6, 2016

Partially it's so that if people create valid nodes they should be able to use them like any other.

The reason I need it is for a CAS step by step simplifier built off of mathJS that Jos is planning to use as an extension to mathjs. I need to merge arguments to simplify easily, and am using my own functions to print and eval, but if the functionality was part of mathJS that'd be awesome

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 6, 2016

I'm not completely sure at this point what the implications for toString and toTex are regarding operator precedence. I will have to think about that in more detail.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 6, 2016

But for now I think it might be okay to just always wrap everything in parentheses.

@evykassirer

This comment has been minimized.

evykassirer commented Nov 6, 2016

I think it's the same as two arguments
Unary minus needs it's argument wrapped in parentheses (and it doesn't make sense for it to have multiple arguments)
Subtraction and division also don't make sense having more than two arguments because e.g. 2-3-4 could be 2-(3-4) or (2-3)-4, you can't "subtract 3 things together"

You can however add or multiply three things together. I could have been more clear that those are the two cases that I care about. They should keep parens around it if it would need it for two arguments, but won't need parens otherwise.

Does that sound right?

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 6, 2016

Yes. Makes sense.

From a toTex and toString perspective this change seems quite feasible.

And as long as an operator is commutative it makes sense to support it in the underlying functions like add and multiply as well.

@josdejong josdejong added the feature label Nov 6, 2016

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 6, 2016

Agree. I think it's nice to extend add and multiply to support more than two arguments. That will basically render sum and prod redundant which is ok (we could keep them as an alias). In the Nodes of the expression parser we only have to do a few changes in toString and toTex. We can keep the parser as is for now (parsing 2+3+4 as two nested additions). In the future we could make the parser smarter if we want. At least, this makes life easier for the step solver that @evykassirer is working on, and could be interesting for @tetslee too (see #734).

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 6, 2016

I think I can do the toString and toTex part of this during the course of next week.

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 6, 2016

👍

I can adjust add and multiply within a few weeks (not sure whether I can make it this week).

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 18, 2016

I've extended add and multiply to accept more than two arguments. Was more tricky than I thought, I first had to fix an old, tough bug in typed-function related to any type parameters.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Nov 18, 2016

This will be very helpful. If I recall, I used nodes named add and multiply in my work for simplify, and I used more than two arguments to convert a nested tree of plus'es and times'es into a flat structure. However, I never attempted to evaluate those functions directly--they were only used for convenience. I always intended to convert back to a nested tree. But this will no doubt be useful.

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 18, 2016

👍

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 18, 2016

This is now available in v3.8.0

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