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

feat: implement symbolicEqual function #2424

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

gwhitney
Copy link
Collaborator

Note for this to work in a broad variety of contexts, has to also allow
identical expressions to cancel regardless of whether subtraction is
always defined; but this seems safe in general, that x-x is 0 even if x
does not generally have an additive inverse (for example, when working
in the positive context).

Resolves #1260.

  Note for this to work in a broad variety of contexts, has to also allow
  identical expressions to cancel regardless of whether subtraction is
  always defined; but this seems safe in general, that x-x is 0 even if x
  does not generally have an additive inverse (for example, when working
  in the positive context).

  Resolves josdejong#1260.
@joshhansen
Copy link
Contributor

I just tried this out (not this code specifically, but the same technique) and am pleasantly surprised. It's an elegant approach.

The weakness seems to be the dependence on simplify, which (IMHO) is not aggressive enough. (I know there's ongoing work on that front.)

But for my current use case (multiplying polynomial factors of a polynomial to see if the original polynomial is yielded) this seems like it will be a great addition. And as simplify grows in capability, the likelihood of false negatives will further diminish.

@gwhitney
Copy link
Collaborator Author

Yes, absolutely, we want simplify to take advantage of as many opportunities as it can. If you have specific examples of expressions that are not being simplified, and what you would like them to simplify to, please please post them as issues. It is a very active area for mathjs right now and the more instances of useful simplifications we have the better mathjs can be in this area. Thanks!

@joshhansen
Copy link
Contributor

@gwhitney I left a comment on #2388 that shows a few cases which are not simplifying for me, even with your new code on that branch.

@gwhitney
Copy link
Collaborator Author

I added them to the tests on that branch, and they seemed to pass OK. Thanks!

@josdejong
Copy link
Owner

Thanks Glen, looks good 👍

I'll solve the merge conflict and merge this PR now.

@josdejong josdejong merged commit c983008 into josdejong:develop Mar 1, 2022
@josdejong
Copy link
Owner

Published in v10.3.0

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

Successfully merging this pull request may close these issues.

Node simplify or deepEqual ordering
3 participants