Skip to content
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

convenient way to define latex for a custom function #276

Closed
FSMaxB opened this issue Feb 11, 2015 · 15 comments
Closed

convenient way to define latex for a custom function #276

FSMaxB opened this issue Feb 11, 2015 · 15 comments
Labels

Comments

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 11, 2015

Currently it is possible to define the Latex output for a custom function as described here in #258 but this isn't very convenient.

It would be nice to be able to just add a toTex-Function to custom functions without having to modify the FunctionNode, like with the rawArgs-Flag. This would also enable using different Scopes with different LaTeX representations for the same function as I understand it.

Example:

customFunctions = {
    divide: function( a, b ) {
        return a/b;
    }
}

customFunctions.divide.toTex = function( a, b ) {
    return '{\frac{' + a + '}{' + b + '}}';
}

math.import( customFunctions );
@josdejong
Copy link
Owner

Thanks, nice idea

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 2, 2015

I tried to implement this and it's kind of possible but not really. I don't even know if it's actually possible to do this properly.

What I'm doing right now:

  • Whenever FunctionNode.prototype._compile gets called, it checks if fn has a function ´toTex´. If so it assigns it to this.customToTex
if (fn && (typeof fn.toTex === 'function')) {
  this.customToTex = fn.toTex;
} else {
  delete this.customToTex;
}
  • FunctionNode.prototype.toTex then checks if this.customToTex exists and if so, calls it.
FunctionNode.prototype.toTex = function() {
  if (typeof this.customToTex === 'function') {
    return this.customToTex(this.args);
  }
    return latex.toArgs(this);
};

The Problem with this approach is, that it requires the FunctionNode to be compiled manually after importing a new scope and that's ugly. The problem is that FunctionNode.prototype.toTex needs access to the custom toTex function somehow. And this function can change whenever a new scope get's imported. The only function that has access to that is FunctionNode.prototype._compile. It might be possible to pass the scope manually when parsing, but the parser shouldn't have to do stuff like that.

Do you have any idea how this could be done properly? Compiling once after the parser is done parsing an expression maybe?

@josdejong
Copy link
Owner

hm. We could:

  1. Pass the math namespace when calling toTex, like toTex(math).

  2. Not attach the custom toTex function to the custom function, but put it them all in a separate, static namespace (like in latex.js) which is accessible by the FunctionNode.prototype.toTex function and made available via for example math.latex. The user then would have to add it's custom function to math.latex.divide, and the toTex function can access this statically. The ugly thing here is that the function is defined globally and not within on math.js instance. Though not ideal, That could be acceptable.

  3. Optionally pass a custom object to the toTex function containing the custom functions, like:

    var node = math.parse('...');
    var myCustomTexFunctions = {
      divide: function (node) {...}
    }
    var tex = node.toTex(myCustomTexFunctions);

Not yet sure, but I may like option 3 most, as there are no "magic" things happening and there are no side effects like possible conflicts in some global namespace.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 3, 2015

The problem with 1 and 3 is, that it isn't transparent to the user. Ideally I'd do an import once and after that toTex just works. 2 has the problem of only one global namespace.

What compile(defs) already does is passing all the definitions down to every single node in the tree. What about doing a similar thing but saving it in every node as this.defs.

Here's what I mean:
Every node would have a function, let's say, setDefs(defs) like this:

//implementing this in Node.js would require every node to follow the convention
//of using this.args to store it's children. Alternatively this function could be
//implemented for every single node separately.
Node.prototype.setDefs = function (defs) {
  this.defs = defs;
  this.args.forEach(function (arg) {
    arg.setDefs(defs);
  });
};

Then, once parsing is done, this function would get called for the root node thereby updating the definitions for every single node in the tree.

A side effect of this would be, that every expression, once parsed, would be completely independent. So you could do a compile without passing a mathjs instance. compile would simply use this.defs instead of getting it passed as argument.

When calling compile with a mathjs instance it would then call setDefs first and after that call compile for it's children so doing compile(math) would still work.

And you could always update the definitions manually by calling setDefs for the root node.

Do you think this is a good idea or is this too invasive?

@josdejong
Copy link
Owner

We could move passing the math namespace from the compile step to the parse step. Then we should pass the math namespace to all Node constructors, and the nodes can store a reference to the math namespace and use it where needed: in the compile step and in the toTex function.

The internal function parse from /lib/expression/parse.js then requires an extra parameter math. The math.parse(expr) function not (it knows it's own math namespace already). And the user doesn't have to provide math to the .compile() step any longer, which thus becomes easier.

Downside is that the Nodes become more coupled with the math namespace, but on the other hand... they already are.

Shall I experiment with this and see if it works out nicely?

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 3, 2015

Nice idea to use the constructors for that. compile should still be able to update the math namespace though, otherwise this would be a breaking change.

Yes, it would be nice if you would experiment with this, but I would also be willing to do it myself if you can't find the time. Mathjs has been a great learning experience so far.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 4, 2015

So lib/function/expression/parse.js needs to call lib/expression/parse.js's parse with math as a parameter:

math.parse = function parse (expr, options) {
  return _parse.apply(_parse, [math].concat(arguments));
}

lib/function/expression/compile.js would do this:

return _parse(math, expression).compile();

and lib/function/expression/eval.js:

return _parse(math, expression).compile().eval(scope);

lib/expression/parse.js's parse function would be like that:

function parse (math, expr, options) {
... ...
}

Every node would then store a reference to math in this.math which get's passed in via the constructor ( which gets called by parse).

Node.prototype.compile would then do something like this:

Node.prototype.compile = function (math) {
  if ((typeof math !== 'undefined') && !(math instanceof ) {
    throw new TypeError('Object expected for parameter math');
  } else if (typeof math === 'undefined') {
    math = this.math;
  }
  ... ...
  ... ...
  var code = this._compile(math, defs);
};

Every nodes _compile would then get math as parameter and store it in this.math.

What I don't really understand right now is what this code does:

var defs = {
  math: _transform(math),
  _validateScope: _validateScope
};

I assume that this code somehow copies parts of the math namespace to a new object.

Maybe the defs object isn't necessary anymore when every node has access to the math namespace. But I don't really understand this in detail. From my understanding, defs would still be there, even if someone decided to do delete math whereas this.math is just a reference to the then deleted object.

Leaving defs there might also lead to unexpected behaviour because if you import new functions, but don't compile the root node, defs would still contain the old functions whereas other stuff like toTex would use the new functions.

This could be solved by copying math before actually storing it in the root node. Then every tree of nodes would have it's own independent copy of the math namespace.

When doing all of this, the only change in mathjs's interface would be that it wouldn't be necessary anymore to pass math to a root node's compile function. Everything else would stay the same, so no breaking change.

This would enable FunctionNode's toTex to access a functions custom toTex via this.math['this.name'].toTex, but this isn't limited to FunctionNode this might be useful for SymbolNode, too. This would make custom toString functions easy to implement as well.

Is my understanding of the matter accurate? And do you think this is something that could be done?

@rjbaucells
Copy link
Collaborator

I have been following the discussion on this thread and I have noticed that changes I am doing to implement Sparse Matrix would affect the solution you are working on (see #275). As part of the change I am implementing Matrix is aware of the "math" instance (actually math.config) it is defined on. As a cascade effect, collection get the same definition:

module.exports = function (math, config) {

var Matrix = ....
};

Following the cascade we get to lib/expression/parse and lib/expression/Parser. Since some of the files in the expression namespace will be defined for a given math instance there will be no need to pass the "math" instance as parameter as you are proposing in this thread.

Maybe it is time to do the change for all files defining types for the math instance. Right now types are statically defined.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 5, 2015

I'm not sure this is really conflicting with what you are doing. The 'only' thing that my proposal does is adding a math argument to lib/expression/parse.js and a math argument for every single constructor of a node. But I don't quite understand what it is you're doing with the math instance to be honest.

Let's wait what Jos has to say about this ( meanwhile I'll continue working on a proof of concept of what I've been proposing, see https://github.com/FSMaxB/mathjs/tree/math-namespace-prototype ).

@rjbaucells
Copy link
Collaborator

It is related, the lib/expression/parse.js definition I have is like:

module.exports = function (math) {
function parse (expr, options) {
...
}
...
return parse;
}

So, there is no need to add the "math" parameter to the parse function as you have in your branch. You can use the local "math" for your needs (from exports function).

The same happens with lib/expression/Parser.js, in this case we can remove the "math" parameter.

see https://github.com/rjbaucells/mathjs/tree/matrix2

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 5, 2015

Now I see what you're doing. This means that math get's passed when loading a module with require and can then be accessed from the local scope.

This is imcompatible with what I've been doing, yes, but my only goal is that I can access math from nodes. So I could rely on either what you're doing or on what I've been doing, it doesn't matter.

I think we need a general solution for passing math around in mathjs, so we need further discussion and we should merge this into mathjs before doing any other work on top of it.

The solution of passing math through as an argument, as I've been doing, is great in that it 'explicitly' passes math around and not 'implicitly' by adding it to the local scope, it isn't a global solution though, only for the parser and the nodes. With your approach it may be possible to make math accessible from everywhere, only problem I see: How would this reference get updated to another instance of math? And in your code you've been replacing require with direct calls to the math namespace, which is kind of inconsistent, we need to work this out properly.

Let's see what Jos has to say, because I'm not familiar enough with mathjs to see the full impact of changes like this.

@josdejong
Copy link
Owner

I've experimented with passing the math instance via the constructors of Nodes. It's terrible. Passing around a math instance everywhere is error prone and gives a high coupling (while Nodes and math where perfectly separated before).

I've in mind that in the future we are going to do algebra, start juggling around with node trees. It's terrible to have to pass the math instance everywhere, makes using the Nodes on a low level quite hard. I think that it's helpful to keep the expression parsing stuff (Nodes, toString, toTex) as good as possible separated from the evaluation stuff (math.*).

Here is the (working) experiment: https://github.com/josdejong/mathjs/tree/namespace_via_constructor

@FSMaxB This would be a breaking change anyway, as the API of the Nodes is public as well. The defs stuff is a helper object, passed when constructing a Function from the generated JavaScript code. The math: _transform(math) is for replacing functions having a transform function attached with this transform function itself.

So to summarize the options:

  • Define custom toTex functions in a global list anyway. Not nice.

  • Pass custom toTex functions or the math namespace as argument, like node.toTex(math) or node.toTex(myCustomTexFunctions).

    • We could make a utility function math.toTex(node) which automatically calls node.toTex(math) under the hood, but that would not really help for usability.
  • Create a factory function for each of the nodes, passing the math namespace as argument, like the parse.js exampe of @rjbaucells:

    module.exports = function (math) {
      function OperatorNode (...) {...}
    
      return OperatorNode;
    }

    We may have to apply the same sort of factory functions for data types, for passing configuration (Like for defining new Units, see Breaking changes for v2 #279 (comment)).

None of these makes me very happy :(

A few more thoughts:

  • For v2 I would like to get rid of this big, shared math namespace used by everything, and limit to passing some simple configuration. This big namespace doesn't help for modularization and separation of concerns: you have no idea what is used by what.
  • Life would be really easy if we would allow only one instance of mathjs. I don't think this is an option though.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Mar 5, 2015

That seems reasonable. Then I wouldn't want to implement custom toTex functions for version 1 and postpone this idea to version 2, because all of the possiblities on how to implement this feature right now aren't really that nice and/or useful in my opinion.

@josdejong
Copy link
Owner

Yes, this will be for v2 I think.

Since we want to support multiple instances of mathjs, I think in the end we need factory functions for (almost) every function and prototype of math.js. The functions needs it, BigNumber needs it, Unit will need it, the Nodes need it, Chain needs it. Etc. Time to think through a neat architecture for this: factory functions, passing configuration, passing constructors of data types. But without this big math namespace which introduces so many implicit, opaque dependencies. We will get there.

@josdejong
Copy link
Owner

We can close this issue I think, the things discussed have been implemented by @FSMaxB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants