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

Cannot pass array into filter() from another function #1088

Closed
LloydLS opened this Issue Apr 23, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@LloydLS

LloydLS commented Apr 23, 2018

Hi,

It looks like filter() can't be used in a custom function, as it won't accept an array passed from outside (unless the array is a predefined variable). Is this expected?

filterNeg(arr) = filter(arr, x > 0 )
filterNeg([-4, 0 , 3, 4 , -2]) // Error: Undefined symbol arr; Expected result: [3, 4]

Tested in version 4.1.2.

Thanks!

@josdejong josdejong added the bug label Apr 23, 2018

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 23, 2018

Thanks for reporting, I can reproduce this bug.

I expect this has something to do with passing the functions arguments to a rawArg function like filter.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jun 1, 2018

I guess the problem here is that the first argument to filter should be a normal/evaluated argument and the second should be a raw arg?

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jun 1, 2018

This patch fixes this problem (although it breaks some tests) by adding an extra value to the scope when filterNeg is evaluated. It is a very hacky solution though.

diff --git a/lib/expression/node/FunctionNode.js b/lib/expression/node/FunctionNode.js
--- a/lib/expression/node/FunctionNode.js
+++ b/lib/expression/node/FunctionNode.js
@@ -2,7 +2,7 @@
 
 var latex = require('../../utils/latex');
 var escape = require('../../utils/string').escape;
-var hasOwnProperty = require('../../utils/object').hasOwnProperty;
+var objectUtil = require('../../utils/object');
 var map = require('../../utils/array').map;
 var validateSafeMethod = require('../../utils/customs').validateSafeMethod;
 var getSafeProperty = require('../../utils/customs').getSafeProperty;
@@ -96,7 +96,9 @@ function factory (type, config, load, typed, math) {
         // "raw" evaluation
         var rawArgs = this.args;
         return function evalFunctionNode(scope, args, context) {
-          return (name in scope ? getSafeProperty(scope, name) : fn)(rawArgs, math, scope);
+          var functionScope = objectUtil.extend({}, scope);
+          objectUtil.extend(functionScope, args);
+          return (name in scope ? getSafeProperty(scope, name) : fn)(rawArgs, math, functionScope);
         };
       }
       else {
@@ -212,7 +214,7 @@ function factory (type, config, load, typed, math) {
   FunctionNode.prototype.toString = function (options) {
     var customString;
     var name = this.fn.toString(options);
-    if (options && (typeof options.handler === 'object') && hasOwnProperty(options.handler, name)) {
+    if (options && (typeof options.handler === 'object') && objectUtil.hasOwnProperty(options.handler, name)) {
       //callback is a map of callback functions
       customString = options.handler[name](this, options);
     }
@@ -370,7 +372,7 @@ function factory (type, config, load, typed, math) {
    */
   FunctionNode.prototype.toTex = function (options) {
     var customTex;
-    if (options && (typeof options.handler === 'object') && hasOwnProperty(options.handler, this.name)) {
+    if (options && (typeof options.handler === 'object') && objectUtil.hasOwnProperty(options.handler, this.name)) {
       //callback is a map of callback functions
       customTex = options.handler[this.name](this, options);
     }
@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 13, 2018

I haven't yet looked into this bug, but I just saw two unit tests in FunctionNode.test.js which are currently skipped which are about this exact issue:

https://github.com/josdejong/mathjs/blob/master/test/expression/node/FunctionNode.test.js#L102-L150

Not sure why I never took the time to get these tests working :(

@josdejong josdejong closed this in f6f7bd2 Jul 7, 2018

@josdejong

This comment has been minimized.

Owner

josdejong commented Jul 7, 2018

This should be fixed now in v5.0.2 (see f6f7bd2)

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jul 7, 2018

Nice work 😄

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