Export the simplify utils as math.simplify.utils

[joshhansen]

This exports the utility functions in math.simplify as math.simplify.utils.

I have found specific use for isCommutative, isAssociative, and flatten and would rather not implement them myself. The others seem like they might also be useful to people, except maybe not createMakeNodeFunction, but for now it's along for the ride.

I can get some Typescript annotations for you as well if you think this is a worthwhile endeavor.

[josdejong]

Thanks @joshhansen . Those functions are really specific for the internals of simplify, I'm not sure whether they are suitable to open up as public functions. Exposing them via simplify.utils is a nice step in between, but still. I totally agree with you though that having to copy the functions if you actually need them is not really an optimal solution.

Can you explain your use case?

[gwhitney]

I'd like to add a +1 to this, specifically for the sake of simplify.utils.flatten. I am using mathjs for translating formulas into canonical strings to use as database keys. The first step is simplify to get (many) equivalent formulas into the same form. The next step will be to flatten -- right now, I have had to re-write my own flatten method; it would be better to use the existing one. Then after that I reorder the arguments to commutative operations in a canonical way (so in fact isCommutative will be useful to have, too!) and finally I convert the whole thing to RPN to produce my final key.

[josdejong]

@gwhitney sounds good to me to expose it.

Before we can merge this PR we have to document simplify.utils. @joshhansen do you still want to address this and update the PR?

[joshhansen]

Hi, I'm still interested in this. I'll add my typescript typings and try to write some docs. I'll add doc comments to the functions involved that lack them, but I take it something more is required to get the docs to generate? I don't see e.g. simplify.utils.unflattenr in docs/reference/functions.md even though it has a doc comment.

Question: why is isCommutative defaulting to true with non-OperatorNode's?

[joshhansen]

Another question: is simplify.utils really where we want these to live? They don't seem very specific to simplify even if that's where they're used in the codebase right now. Maybe just under utils?

[gwhitney]

Since I know Jos has limited time, I'll take a stab at these latest questions; of course, my responses are just attempts to help and express opinions, not authoritative.

I'll add doc comments to the functions involved that lack them, but I take it something more is required to get the docs to generate?

Have you tried npm run build:docs ?

Question: why is isCommutative defaulting to true with non-OperatorNode's?

I do not know. I can confirm from my work on #2391 that changing this default to false does not break any unit tests. But that doesn't really answer your question.

Another question: is simplify.utils really where we want these to live? They don't seem very specific to simplify even if that's where they're used in the codebase right now. Maybe just under utils?

Well, math.flatten is the function that takes a 2D (or higher-dimensional array) and turns it into a 1D array. I think it would seem odd for just "flatten" to deal with arrays and "utils.flatten" to deal with parse trees. And it seems to me the behavior of the functions is different enough that they should not be collapsed into the same Typed function. So that means the expression-tree 'flatten' is going to need some prefix that indicates what realm it lives in. And since all of these are defined together, it seems like they should all have the same prefix, that should indicate their syntax-related nature. Given those considerations, the "simplify.utils" prefix seems as good as any to me, although something like "syntax.utils" or "expression.utils" seems like it would be fine as well -- just neither "syntax" or "expression" is currently used as a prefix, whereas "simplify" is, so why add a new one?

[josdejong]

Since I know Jos has limited time

Yes true, and frustrating at times. But some big news: starting March 1st, I'll start working full time on my open source projects 🎉 . Can't wait for it 😁

Another question: is simplify.utils really where we want these to live?

Good question. So far, we try to have a flat list with functions in mathjs, and not nested things like simplify.utils.unflattenr. These util functions started as internal functions, and now get more important. So at this point I think we can go two directions:

1. keep them as second class citizen for now, exposed under simplify.utils.*. We can keep documentation minimal: add a section in docs/expressions/algebra.md mentioning the existence of these functions.
2. promote them into first class functions. That means splitting the source code in separate files, exposing as math.*, adding extensive unit tests for the functions, adding guarding on correct usage probably, thinking through the API and edge case of these now public functions (like thinking through a solution for having two flatten functions), thinking through the naming, writing detailed docs for each of the function, etc. We could also make some of the util functions public and not all.

Question: why is isCommutative defaulting to true with non-OperatorNode's?

I have no idea either. I guess we can change it if it doesn't make sense and doesn't break any existing behavior.

[gwhitney]

On npm run build:docs and the "location" of these functions:

1. build:docs doesn't recognize updates to the source files. To get stuff from changes to the doc comments in the source, you have to run a full npm build.

2. In the primary develop branch, build:docs only tries to generate docs for items at the "top level" in the mathjs object, i.e., math.rationalize but not e.g. math.simplify.resolve. PR fix: simplify.resolve detects reference loop and throws error #2405 extends this -- but only one more level down, so it gets math.simplify.simplifyCore but would not get something like math.simplify.utils.isCommutative. So either doc generation would have to be extended one more level, or as Jos suggests in option 1. in the last comment, they would only be documented inside another documentation file, or they could be moved up in the hierarchy as suggested in option 2. If fix: simplify.resolve detects reference loop and throws error #2405 is accepted, then there would be the option to move them up just one level rather than all the way to the top, e.g., they could be simplify.flatten, simplify.isAssociative, etc., to be siblings of the previously existing simplify.resolve, which the PR makes to be pretty much full-fledged like any top-level mathjs function.

[josdejong]

@gwhitney how do think about keeping all functions of mathjs in a single, flat list? So not math.simplify.utils.isCommutative but math.isCommutative? That is how the library is created so far, and I have a slight preference for that approach. When growing, it could be that we start to get naming conflicts, like with flatten which could mean different things. But we could use naming prefix like math.simplifyCore, math.simplifyFlatten (and we have something similar already with the Set functions).

[gwhitney]

Your desired naming convention seems fine to me, and I think your judgment should be primary in this sort of thing. In that case, I'd suggest being conservative as to which of the utils move out to the top level: definitely flatten, isCommutative, isAssociative. And then yes, flatten needs a new name. I'd suggest flattenAssociation, since it should really only apply to associative operators, and mentioning association hints that this is something that deals with expressions, and this flatten doesn't inherently have to do with simplify, which is a strike against simplifyFlatten. But it's just a suggestion, you should decide on the final name.

[josdejong]

Thanks, sounds good 👍 . The names you suggest make sense to me: flattenAssociation, isCommutative, and isAssociative.

[joshhansen]

Alright, so, summarizing:

• math.simplify.utils.flatten -> math.flattenAssociation
• math.simplify.utils.isAssociative -> math.isAssociative
• math.simplify.utils.isCommutative -> math.isCommutative, defaulting to false if that doesn't break anything
• math.simplify.utils.unflattenr -> math.unflattenr
• math.simplify.utils.unflattenl -> math.unflattenl
• math.simplify.utils.createMakeNodeFunction -> I propose that this be omitted, unless someone wants to speak in favor of exposing it

As for the location in the source, src/function/utils seems like a good home.

Some of these mutate their arguments in place, which goes against the grain of e.g. math.rationalize and math.simplify. We could provide a non-mutating wrapper if that's a concern.

I'll get to work on the refactor!

[joshhansen]

Tangentially: @gwhitney Your use case and mine sound fairly similar. We are doing a lot of canonicalizing of expressions and equations, for equality comparison purposes. I wonder if we might want to collaborate, maybe build an adjunct library to mathjs. Unless mathjs wants to host canonicalization functionality? @josdejong

[gwhitney]

Responding:
To summary of changes: isCommutative has already changed to default false in develop.
I'd suggest for exposing in the public interface, unflattenl and unflattenr should be merged into a single function unflattenAssociation (to be parallel to flattenAssociation) defaulting to the association of how a+b+c+d is parsed, with a keyword argument for the other association. (Just doesn't seem important enough to take up two function names.)
One gotcha that got me at first is that once you put these at top level, simplify should just list them as dependencies rather than load them up explicitly.

On canonicalize - (1) if all you are after is testing expression equality, I have read that simplifying the difference to 0 is often a more effective method, so you might want to try symbolicEqual once it has hit develop. (2) in my case I don't have that luxury of just testing equality because I need a string for a database key, so I do need the best canonicalize I can manage. If you really need it too, I don't think it would be out of place among the algebra functions, and my guess is Jos won't mind, either. So I'd be happy to collaborate on getting a canonicalize in.

[gwhitney]

P.S. on the summary of changes -- I should think that once they are top-level functions, those that are should have their source directly in src/functions/algebra.

And I agree, for consistency with everything else in mathjs, public functions should not mutate their arguments in place. So perhaps the top-level functions are just wrappers on things from utils, and the utils remain mostly in place (in which case if simplify is just using the util versions, it could continue to load those directly rather than registering a dependency on the top level versions).

[josdejong]

Thanks guys, for all the inputs and work 👍

On building an adjunt library to mathjs for canonicalization: I have the feeling that the current simplification functionality fits just fine in mathjs. I can imagine thought that the CAS system grows bigger and bigger and at some point becomes a thing on it's own. If there are good reasons to split it into a separate repository we could consider that then, just like Eric is working on a standalone units library https://github.com/ericman314/UnitMath that hopefully will replace the older Unit functionality. Feel free to open a separate discussion on separating the CAS functionality from mathjs if you feel that has good benefits.

[gwhitney]

@joshhansen is this still on your plate to wrap up at some point? As far as I can see the outstanding issues are:

• Make these functions not modify their arguments by side effect, or make wrappers that don't
• Rename the functions and relocate the source code as appropriate
• Make sure they are available in the parser and that they have embedded docs
• Make sure doc generation is working (I think it will be)
• Make sure they all have unit tests (may already be done)

Feel free to check off anything that is done and/or add any tasks I have missed.

[gwhitney]

As I ultimately need these functions exposed, I am willing to complete the task list above to get this ready to merge.

[josdejong]

Thanks Glen!