feat(simplify): Provide context option to control simplifications allowed#2399
feat(simplify): Provide context option to control simplifications allowed#2399josdejong merged 10 commits intojosdejong:developfrom
Conversation
|
I see the Node 12 and browser test failures. They are both occurring because in my new tests that values are preserved when simplifying, there are some expressions in which both the original and simplified forms produce NaN (those are critical tests; it's exactly simplifications that make NaN become a regular number that realContext is trying to avoid). But in Node 12 and browsers, NaN is not deepEqual to NaN, whereas in Node 14 and Node 16 it seems that it is. Can you let me know what assertion I can use that will be OK in all settings so that as long as both expressions produce NaN, or both produce the same regular number, or both produce equivalent arrays or objects (which will not be ===), then the assertion will pass? Thanks. |
|
Thanks Glen! I haven't yet looked into the PR itself, but about the NaN issues: so far we just use |
|
Thanks, checking explicitly with isNaN() has cleared up the tests on all platforms. |
josdejong
left a comment
josdejong
left a comment
There was a problem hiding this comment.
Wow, really impressive piece of work Glen 😎. How you pass the new context everywhere is quite straightforward. And I really love how readable and understandable the new assuming API reads, like:
{
s: '-(c*v) -> v * (-c)', // make non-constant terms positive
assuming: { multiply: { commutative: true }, subtract: { total: true } }
}I made a couple of minor inline comments, but overall it's a solid piece of work and looks well tested💪 .
As a last thing: do we need to write a page/section in the documentation explaining how the context works on a high level? Maybe on the Algebra page? Or is the documentation on the simplify function itself enough?
If the options argument has a key 'context', it value is interpreted as a context specifying various (non-default) properties of operators. This context is propagated to all rules and all matching. Adds some initial tests that the context option affects the behavior of simplify appropriately. Not all can be activated until in a future commit we add the ability for the application of a rule to be contingent on aspects of the context. Note that the enhanced rule matching necessary to support rules constrained by non-default operator properties led to a couple of changes to the output of rationalize() as well. Since the new output seemed to better match what a person would typical write for the rationalized form, this commit changed the test rather than attempted to preserve the exact prior order of terms.
Prior to this commit, simplifyCore stripped internal parentheses, but would leave top-level ones. But top-level parentheses don't carry any semantics, and no tests other than the ones that explicitly checked for the retention of top-level parentheses were affected by this change. Not making a special case for the top level also notably streamlined the code in simplifyCore. Adds tests for the new parenthesis-stripping behavior, as well as for other node types that were added earlier but which did not yet have simplifyCore tests.
This replaces special-case tests for unary + and parentheses, and paves the way for example for 'abs' being marked trivial in a putative positiveContext
The new name is 'imposeContext' -- the motivation for the change is to distinguish the parameter for 'assuming', which will be added as a new parameter to control rule application based on context.
Adds a new property of rules specified as objects: `assuming`. Its value should be a context, and every property specified in that context must match the incoming context, or else the rule will not be applied. Updates the constant floating rules to require their operators be commutative, as a test of the feature, and adds a unit test for this.
Also activates a number of tests of simplifications that should or should not occur in various contexts. To get all tests to pass, I could no longer find a rule ordering that worked in all cases, without the ability to mark an individual rule as applying repeatedly until just that rule stabilized. So this commit also adds that ability, and uses it to eliminate the tricky rule of expanding n1 + (n2 + n3)*(-1) to n1 + n2*(-1) + n3*(-1) late in the rule ordering, in favor of the more intuitive (complete) expansion of (n1 + n2)*(-1) to n1*(-1) + n2*(-1) early in the rule ordering, before constant folding and gathering of like terms.
In particular, adds a `simplify.realContext` and a `simplify.positiveContext` which (attempt to) guarantee that no simplifications that change the value of the expression, on any real number or any positive real number, respectively, will occur. Adds multiple tests for these contexts, including verification that the simplification in either context does not change some example values of any of the expressions in any simplify test. This testing uncovered that it is unaryPlus that must be marked as trivial for simplifyCore to work properly, so that marking is added as well.
The problem was NaN != NaN in some JavaScripts but not others, so test for "both values NaN" explicitly before using deepEqual.
gwhitney
force-pushed
the
simplify_with_context
branch
4 times, most recently
from
164ff5a to
c002414
Compare
Added documentation about scope and context in top-level algebra functions page; made variable name less abbreviated; performed suggested refactoring.
gwhitney
force-pushed
the
simplify_with_context
branch
from
c002414 to
2f78d1b
Compare
|
Sorry for all of the pushes, I didn't know of any other way to preview changes to "algebra.md" besides pushing it and looking at it in my fork on GitHub. In any case, I think I have addressed all of your comments. Thanks for your encouragement in this (large-ish) change! Hopefully this is ready to merge or nearly so. |
|
Great Work |
gwhitney
changed the title
|
Thanks for all your updates, this is a solid foundation on which we can expand further 💪 No problem at all about all the commits, I'll squash it all in one anyway when merging 😁 |
|
Published in |
3 participants
Overall, this PR implements a new option to simplify,
context, which lists properties that various operators should be assumed to have or not have during the simplification process. The default context allows all previously existing simplifications to occur. Two alternate contexts are provided,simplify.realContextandsimplify.positiveContext, which are guaranteed to preserve the values of expressions on all reals and all positive reals, respectively. Or a custom context with specific operator properties can be passed in, and the simplify rules are tagged with the properties they depend on so they will only be applied if appropriate. Multiple tests for the new option are supplied. (However, even with these tests and the best effort to be complete, it is likely that some rules could still use further annotation, which should hopefully be easy to add if/when simplification anomalies come up.)Resolves #2391.
This is clearly a larger-than-usual PR. It is the smallest PR that I could come up with that addressed all of the points in the issue it attempts to resolve. However, given that it is large, I am perfectly willing to break it up and submit as a sequence of two or more PRs. If that is preferred, please just let me know what pieces you recommend I break it into. Thanks.