Prevent RecursionError in nc_simplify for floating point coefficients #26557

rrodenbusch · 2024-04-30T13:44:06Z

References to other Issues or PRs

Fixes #26556

Brief description of what is fixed or changed

Allow simplification of expressions with floating point coefficients for non-commutative symbols

Other comments

Release Notes

simplify
- Prevent RecursionError when simplifying expressions with floating-point coefficients on a non-commutative symbol

sympy-bot · 2024-04-30T13:44:09Z

✅

Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

simplify
- Prevent RecursionError when simplifying expressions with floating-point coefficients on a non-commutative symbol (#26557 by @rrodenbusch)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #26556

#### Brief description of what is fixed or changed
Allow simplification of expressions with floating point coefficients for non-commutative symbols 

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* simplify
   * Prevent RecursionError when simplifying expressions with floating-point coefficients on a non-commutative symbol
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2024-04-30T13:56:24Z

sympy/simplify/simplify.py

@@ -1758,7 +1758,7 @@ def compare(s, alt_s):
        # get the non-commutative part
        c_args, args = expr.args_cnc()
        com_coeff = Mul(*c_args)
-        if com_coeff != 1:
+        if com_coeff != 1 and com_coeff != 1.0:


We might get floats of different precision.

It would be better to use from sympy.core.numbers import equal_valued.

oscarbenjamin · 2024-04-30T14:22:24Z

.mailmap

@@ -1654,3 +1654,4 @@ zzj <29055749+zjzh@users.noreply.github.com>
 Łukasz Pankowski <lukpank@o2.pl>
 彭于斌 <1931127624@qq.com>
 袁野 (Yuan Ye) <yuanyelele@tutanota.com>
+rrodenbusch <rrodenbusch@gmail.com>


You will need to rerun bin/mailmap_check.py to sort the file.

oscarbenjamin · 2024-04-30T14:53:50Z

Don't worry about the failing mpmath tests (mpmath/mpmath#704 (comment))

oscarbenjamin · 2024-04-30T14:59:03Z

.mailmap

@@ -1616,6 +1616,7 @@ richierichrawr <richierichrawr@users.noreply.github.com>
 rimibis <33387803+rimibis@users.noreply.github.com>
 risubaba <risubhjain1010@gmail.com>
 ritikBhandari <ritikbhandari68@gmail.com>
+rrodenbusch <rrodenbusch@gmail.com>


Usually the idea is to add a .mailmap entry like:

Richard Rodenbusch <rrodenbusch@gmail.com> rrodenbusch <rrodenbusch@gmail.com>

The first name and email address is what will eventually go in the AUTHORS file. The second is what is recorded in the commit metadata. This line in the .mailmap file associates the author name with the corresponding commits.

There are a number of entries not like that in the .mailmap file that mostly come from people who have misunderstood what these entries are intended to do.

I will update my entry. The entry in the documentation for this process mentions the dual entry format after the step to commit the change to .mailmap. Perhaps we should re-order that?

Maybe. I'm not sure which part you are referring to. Is it this:
https://docs.sympy.org/dev/contributing/new-contributors-guide/workflow-process.html#add-your-name-and-email-address-to-the-mailmap-file

Yes. That is the document I was following. I'd tweak the document so it shows the two entry option in the initial section, then links to the section below with the more detailed description on the entries. I'm attaching changes here so you can look at it before I add it to the PR.

first section

The first time you make a pull request you will need to add your name and email address to the .mailmap file by adding a line like

Joe Bloggs <joe@bloggs.com> joeb <joe@bloggs.com>

This line in the .mailmap file associates the author name with the corresponding commits. The first name and email address is what will eventually go in the AUTHORS file. The second entry is what is recorded in the commit metadata. (see Mapping user names to AUTHORS file entry)

The commit metadata name and email should exactly match the name and email that you have configured with git before making the commits (see "Configure git settings" above). The bin/mailmap_check.py script can check that this has been done correctly.

then below

(mailmap-mapping-names)=

Mapping user names to AUTHORS file entry

Sometimes a commit will be made with an incorrect name or email address or an author will make multiple commits with different names and email addresses or an author wishes to use a proper name that differs from their github name. In this case a line should be added to the .mailmap file where the first name and email address is what should be recorded in the AUTHORS file and the others are the name and email address that was incorrectly used in the other commits.

the updated md

workflow-process.md

Yes, maybe that is better.

The premise of the way it is written currently is that you will configure git to have the right name and email address first:

$ git config user.name Oscar Benjamin

It seems that it refers to "configure git settings" that is not present in the document though.

The two entry version is only needed if the name configured in git is not the name that should be in the authors file. It seems to be very common though that the name and/or email in the commits is not the right one for the authors file so it is probably more common to need the two entry version. I agree that

Joe Bloggs <joe@bloggs.com> joeb <joe@bloggs.com>

is probably the best thing to suggest by default.

I found 'configure git settings' in dev-setup.md and added the links. Do I add these changes be added to this PR or should I generate a new one?

workflow-process.md

dev-setup.md

I'd say go with a new PR for the workflow changes.

In the new PR you won't have the problems because you are already in the .mailmap file (unless you change your git config...).

oscarbenjamin · 2024-04-30T20:30:25Z

Looks good. Thanks!

rrodenbusch · 2024-04-30T21:06:42Z

I will submit the documentation changes in another PR. Maybe we can help those who follow avoid the mistake I made.

…

On Tue, Apr 30, 2024, 15:31 Oscar Benjamin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .mailmap <#26557 (comment)>: > @@ -1616,6 +1616,7 @@ richierichrawr ***@***.***> rimibis ***@***.***> risubaba ***@***.***> ritikBhandari ***@***.***> +rrodenbusch ***@***.***> In the new PR you won't have the problems because you are already in the .mailmap file (unless you change your git config...). — Reply to this email directly, view it on GitHub <#26557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE3FKCYJ6BVP4PTKSB2X4WDY775RXAVCNFSM6AAAAABHAJZSS6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZSGQ3DMOJVHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Prevent RecursionError in nc_simplify for floating point coefficients

29f686c

oscarbenjamin added the simplify label Apr 30, 2024

oscarbenjamin reviewed Apr 30, 2024

View reviewed changes

rrodenbusch added 2 commits April 30, 2024 08:59

First time contributor

93baa1f

Use sympy.core.number.equal_valued for coefficient test

14f3199

oscarbenjamin reviewed Apr 30, 2024

View reviewed changes

Add name to mailmap file

07f5e42

oscarbenjamin reviewed Apr 30, 2024

View reviewed changes

author: add Richard Rodenbusch to .mailmap

e3c8666

oscarbenjamin merged commit bd0b3e6 into sympy:master Apr 30, 2024
44 of 48 checks passed

rrodenbusch deleted the _simplify-recursion branch May 9, 2024 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent RecursionError in nc_simplify for floating point coefficients #26557

Prevent RecursionError in nc_simplify for floating point coefficients #26557

rrodenbusch commented Apr 30, 2024

sympy-bot commented Apr 30, 2024 •

edited

Loading

oscarbenjamin Apr 30, 2024

oscarbenjamin Apr 30, 2024

oscarbenjamin commented Apr 30, 2024

oscarbenjamin Apr 30, 2024

rrodenbusch Apr 30, 2024

oscarbenjamin Apr 30, 2024

rrodenbusch Apr 30, 2024

oscarbenjamin Apr 30, 2024

rrodenbusch Apr 30, 2024

oscarbenjamin Apr 30, 2024

oscarbenjamin Apr 30, 2024

oscarbenjamin commented Apr 30, 2024

rrodenbusch commented Apr 30, 2024 via email

Prevent RecursionError in nc_simplify for floating point coefficients #26557

Prevent RecursionError in nc_simplify for floating point coefficients #26557

Conversation

rrodenbusch commented Apr 30, 2024

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

first section

then below

Mapping user names to AUTHORS file entry

the updated md

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Apr 30, 2024

rrodenbusch commented Apr 30, 2024 via email

sympy-bot commented Apr 30, 2024 •

edited

Loading