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

function arguments not available in `scope` #790

Closed
hastela opened this Issue Feb 2, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@hastela

hastela commented Feb 2, 2017

Hi,
i am trying to implement my own custom sum function.
The goal is to have a function called "sumFromTo" that takes 4 parameter

  • iteratorname
  • from
  • to
  • function

where "function" can be some other function and where the solution is to sum all.
For example

// works
sumFromTo(i,1,10,i+1)  // = 65

But if i wrap this into an other function it will break, because mathjs doesn't know "x"

// works not because x is undefined
test(x) = sumFromTo(i,1,x,i+1) 
test(10)

My Import looks like this:

function sumFromTo(iteratorName, from, to, f) {
        var total = 0;
        var toVal = typeof to == "number" ? to : to.length;
        for (var iterator = from; iterator <= toVal; iterator++) {
            mathScope[iteratorName] = iterator;
            total += math.eval(f, mathScope);
        }
        return total;
    }
    sumFromTo.transform = function (args, math, scope) {
        /*  get Iterator as String     */
        var iteratorName = null;
        if (args[0] instanceof math.expression.node.SymbolNode) {
            iteratorName = args[0].name;
        } else {
            throw new Error('First argument must be a symbol');
        }

        /*    Startvalue of Iterator    */
        if (args[1] instanceof math.expression.node.ConstantNode) {
            if (args[1].value == 0) {
                throw new Error('Sum must counting from >=1: 0 given');
            }
        }

        /*    compile    */
        var from = args[1].compile().eval(scope);
        var to = args[2].compile().eval(scope);
        
        if (to.constructor.name == "Matrix") {
            to = to.toArray();
        }else{
            if(typeof to == "object"){
                to = to.toArray();
            }
        }
        return sumFromTo(iteratorName, from, to, args[3].toString());
    };
    sumFromTo.transform.rawArgs = true;
    math.import({sumFromTo: sumFromTo}, {override: true});

I create a fiddle https://jsfiddle.net/o49p4zwa/2/ , maybe it's helpful to understanding my problem.

Does someone know what i am missing or what i am doing wrong?
Thank in advance!!

@hastela

This comment has been minimized.

hastela commented Feb 3, 2017

Ok i think the reason is that i use

sumFromTo.transform.rawArgs = true;

If i remove this line the "x" is known in sumFromTo, but then i cant overwrite "i" in the sum function.
If i use another variable, for example "a" so that my line looks like

test(x) = sumFromTo(a,1,x,a+1) 

then math.js throw me an error, because "a" is undefined.

Does someone know how to fixed that? Or maybe a workaround for this special case?
Thanks

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 3, 2017

Thanks for reporting this, this is a bug in math.js. I don't think there is a workaround but I'll fix it soon.

Reason is function arguments (like x in test(x) = ...) are treated separately and are not added to scope (for better performance), that needs to be fixed.

@josdejong josdejong added the bug label Feb 3, 2017

@josdejong josdejong changed the title from custom function support to function arguments not available in `scope` Feb 3, 2017

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 3, 2017

It's quite easy to fix this, I can do it this weekend I think.

@hastela

This comment has been minimized.

hastela commented Feb 4, 2017

Oh, nice to hear! That would be awesome!

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 5, 2017

This should be fixed now in the develop branch. We will do a bug fix release within a few days.

If you can't wait you can clone and build the library yourself to verify whether the fix works :D

@hastela

This comment has been minimized.

hastela commented Feb 5, 2017

Whoa! It works! 👍
Thank you very much!!

@hastela hastela closed this Feb 5, 2017

@hastela hastela reopened this Feb 5, 2017

@hastela

This comment has been minimized.

hastela commented Feb 5, 2017

Sorry, it does not work completely. Or i forgot something in my implementation of my sum function.

//works
test(x) = sum(j,1,x,1) 
test(10) // return 10

// works not
test(x) = sum(j,1,10,x) 
test(1) // should return 10

It throws an exception with "Undefined symbol x" again :-/
Any idea?

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 5, 2017

Thanks for the feedback. What is the exact code of your function sum? (so I can reproduce the issue)

@hastela

This comment has been minimized.

hastela commented Feb 5, 2017

It is the same as in my first post.
I think it is because in the transform function i return

return sumFromTo(iteratorName, from, to, args[3].toString());

so the "normal" sumFromTo Function gets a string with that undefined "x" and try to eval it. But in my current implementation i need a string and cant compile that in the transform function, because the loop in my normal sumFromTo function would only add the precompiled value instead of the content of the third parameter. I hope it is understandable what i mean. My english ... :-/

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 5, 2017

I think the reason is that in your use case test(x) = sum(j,1,10,x), x is the third argument, then in sumFromTo.transform you call

return sumFromTo(iteratorName, from, to, args[3].toString())

this args[3].toString() resolves to the string 'x', which is the argument f in function sumFromTo:

  function sumFromTo(iteratorName, from, to, f) {
    // ... 
    total += math.eval(f, mathScope);
    // ...
  }

but there you simply evaluate math.eval('x', mathScope), and this mathScope does not contain the scope of this variable x, it's a different scope than the scope passed to sumFromTo.transform.

To solve this issue, what you could do is instead of simply doing args[3].toString(), you could first resolve all known variables of args[3]. This works:

<!DOCTYPE html>
<html>
<head>
  <title>#790</title>
  <script src="../dist/math.js"></script>
</head>
<body>
<script>
  // see: https://github.com/josdejong/mathjs/issues/790

  var mathScope = {};

  function sumFromTo(iteratorName, from, to, f) {
    var total = 0;
    var toVal = typeof to == "number" ? to : to.length;
    for (var iterator = from; iterator <= toVal; iterator++) {
      mathScope[iteratorName] = iterator;
      total += math.eval(f, mathScope);
    }
    return total;
  }

  sumFromTo.transform = function (args, math, scope) {
    /*  get Iterator as String     */
    var iteratorName = null;
    if (args[0] instanceof math.expression.node.SymbolNode) {
      iteratorName = args[0].name;
    } else {
      throw new Error('First argument must be a symbol');
    }

    /*    Startvalue of Iterator    */
    if (args[1] instanceof math.expression.node.ConstantNode) {
      if (args[1].value == 0) {
        throw new Error('Sum must counting from >=1: 0 given');
      }
    }

    /*    compile    */
    var from = args[1].compile().eval(scope);
    var to = args[2].compile().eval(scope);

    if (to.constructor.name == "Matrix") {
      to = to.toArray();
    } else {
      if(typeof to == "object") {
        to = to.toArray();
      }
    }

    /*   resolve variables in scope  */
    var f = args[3].transform(function (node, path, parent) {
      if (node.isSymbolNode && node.name in scope) {
        return new math.expression.node.ConstantNode(scope[node.name]);
      }
      else {
        return node;
      }
    });

    return sumFromTo(iteratorName, from, to, f.toString());
  };
  sumFromTo.transform.rawArgs = true;

  math.import({sumFromTo: sumFromTo}, {override: true});

  var scope = {};
  math.eval('test(x) = sumFromTo(j,1,x,1)', scope);
  console.log(math.eval('test(10)', scope));  // outputs 10

  math.eval('test2(x) = sumFromTo(j,1,10,x)', scope);
  console.log(math.eval('test2(1)', scope));  // outputs 10
</script>
</body>
</html>
@hastela

This comment has been minimized.

hastela commented Feb 5, 2017

Thanks for your help!!
I think i understand it a bit more

So if "x" is not only a variable or symbol i need more cases to test and to add the content of the string "f" to the scope?

image

So, if i understand it right, i have to test what kind of "f" is and then add all to the scope?

Is there an easier way, because i think for example a sum in a sum in a sum with different symbols and so on would be a lot of testing in the transform function :-D

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 6, 2017

I'm not sure whether I understand you, but this shouldn't be needed, the code I added to resolve variables in scope should just do it.

The cause of the issue is that you first do a math.eval(..., scope) where the expression contains your special function with rawArgs, and inside that you call math.eval(..., mathScope). The scope of the first and second eval are simply two separate things, mathScope does not contain the variables of scope.

To solve that there are two options:

  1. resolve the variables of the first scope before passing the expression to the second eval with different scope (that's what I did in the last solution)
  2. merge the variables of scope into mathScope before evaluating the second eval.

@josdejong josdejong closed this in f8370bd Feb 6, 2017

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 6, 2017

I've just released v3.9.1 containing this fix.

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