fix(simplify): Array and object simplification#2382
Conversation
Mostly ArrayNode, ObjectNode, AccessorNode, and IndexNode nodes are simply transparent to simplification -- they simply allow it to occur within subexpressions. Then main exception is that if an array or object is indexed by a constant, the expression can be replaced by the corresponding subitem, e.g. `[3,4,5][2]` simplifies to `4`. This at least partially resolves josdejong#1913 (see my latest comment there).
This involves allowing ArrayNodes containing only constant entries to temporarily convert to Matrix type inside of simplifyConstant, so that function and operator calls can occur on them. I also had to add a special case for the function `size` because it can be computed even on symbolic arrays, since the result depends only on the shape, not the entries. Deals with additional cases of josdejong#1913; unclear if there are remaining aspects of that issue on which further work is desirable.
And restores inadvertent deletion of a blank line.
|
|
||
| /* Handles constant indexing of ArrayNodes, matrices, and ObjectNodes */ | ||
| function _foldAccessor (obj, index, options) { | ||
| if (!isIndexNode(index)) { // don't know what to do with that... |
There was a problem hiding this comment.
@gwhitney I see your comment, does this case still need attention, or is the current behavior ok?
There was a problem hiding this comment.
Well, I don't even know what the semantics are if the index member isn't an IndexNode object. (I don't even know if it could possibly occur that the index is not an IndexNode object.) So given that, there's no possible simplification I could think of performing. That was supposed to be the import of the comment. So let me know whether to leave the comment, delete it, or replace it with // is this even possible?
There was a problem hiding this comment.
I think your implementation is correct. When using an expression parsed with the expression parser I don't think it can occur that index is not an IndexNode. Maybe when someone has manually created all the nodes and simplifies that it could accidentally occur.
Thinking about it, I think you can just leave the comment as is: when the index is not an index node, it makes sense that simplify cannot know what to do with it, and best is to just leave it as is.
There was a problem hiding this comment.
OK, then is there anything else I need to do for this PR so that you can merge it? Thanks for creating mathjs, by the way, it's a big help to our OEIS visualization project.
There was a problem hiding this comment.
no all is ready to merge, I'll click the button 👍
There was a problem hiding this comment.
Thanks for creating mathjs, by the way, it's a big help to our OEIS visualization project.
Thanks, good to hear 😄
|
Impressive piece of work Glen, thanks a lot. It is really awesome to see expressions like I made one minor comment, but all looks good to merge 👍 |
|
Published now in |
|
Well, simplify is critical to our project as we use it to canonicalize sequence expressions for lookup/caching in our database of sequences. So in fact, you ain't seen nothin' yet ;-) (well, I mean just that there are still a number of ways we're hoping to enhance it). |
|
Ha ha, that sounds promising 😄 . I'll try to keep up with you (but all is spare-time work on my side, time is limited). |
Allows simplification of expressions inside arrays, objects, and indices; handles operations on constant arrays.
Partially or possibly fully addresses #1913.