Fix rewrite(abs) to call rewrite(Abs) #27325

Ytemiloluwa · 2024-11-28T08:06:15Z

References to other Issues or PRs

Brief description of what is fixed or changed

This PR resolves issue #27323 by ensuring that rewrite(abs) correctly maps to rewrite(Abs) for consistency when using the built-in abs function. @asmeurer @oscarbenjamin

Other comments

Changes Made:

Updated the rewrite method to handle abs as a special case, redirecting it to Abs.
Added a test case in sympy/core/tests/test_basic.py to confirm that rewrite(abs) produces the same output as rewrite(Abs).

Release Notes

core
- Fixed rewrite(abs) to call rewrite(Abs), ensuring consistent behavior.

sympy-bot · 2024-11-28T08:06:22Z

✅

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:

core
- Fixed rewrite(abs) to call rewrite(Abs), ensuring consistent behavior. (#27325 by @Ytemiloluwa)

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

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 #27323
#### Brief description of what is fixed or changed
This PR resolves issue #27323 by ensuring that rewrite(abs) correctly maps to rewrite(Abs) for consistency when using the built-in abs function. @asmeurer @oscarbenjamin 

#### Other comments
Changes Made:

- Updated the rewrite method to handle abs as a special case, redirecting it to Abs.
- Added a test case in sympy/core/tests/test_basic.py to confirm that rewrite(abs) produces the same output as rewrite(Abs).

#### 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 -->
* core
     * Fixed rewrite(abs) to call rewrite(Abs), ensuring consistent behavior.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

anutosh491 · 2024-11-29T11:04:18Z

sympy/core/tests/test_basic.py

+def test_rewrite_abs():
+    from sympy import Piecewise, Eq
+    assert sign(x).rewrite(abs) == sign(x).rewrite(Abs)
+    assert sign(x).rewrite(abs) == Piecewise((0, Eq(x, 0)), (x / Abs(x), True))


Usually you might have seen tests in SymPy being framed as test_issue_xx where xx is the issue number, but something like this being more fundamental we could use test_rewrite_abs.

My only suggestion here would be to add the issue link either

"# issue 27327"

Or even the whole issue link

Because in future we should be able to trace back why this was added and having a comment makes it easier.

sympy/core/tests/test_basic.py

anutosh491 · 2024-11-29T14:12:38Z

sympy/core/tests/test_basic.py

+
+    Related to issue 27327: https://github.com/sympy/sympy/issues/27327
+    Ensure rewrite(abs) calls rewrite(Abs) and produces expected results.
+    """


Hmm I guess a link to the issue is enough :)

Should I remove the comments?

I mean yeah, a link to the issue should be enough.

Okay I will do that.

anutosh491

Approving but I'll still wait for reviews by other maintainers .... Just to make sure this is the best place to add the condition we're interested in !

Ytemiloluwa · 2024-12-01T08:20:38Z

Approving but I'll still wait for reviews by other maintainers .... Just to make sure this is the best place to add the condition we're interested in !

Okay thank you. Hello @oscarbenjamin please can you take a look at this PR, it has been approved and it is ready to merge.

oscarbenjamin · 2024-12-01T11:52:06Z

Are there any other functions analogous to abs where we would want to do this?

Ytemiloluwa · 2024-12-01T12:38:35Z

Are there any other functions analogous to abs where we would want to do this?

Thank you for your suggestion! The _eval_rewrite_as_Abs method already exists for both Max and Min in sympy/functions/elementary/miscellaneous.py. These functions rewrite themselves in terms of Abs.

To confirm this functionality, I’ve written tests to verify their behavior and they all passed:

Basic cases for Max and Min with two arguments.
Multi-argument cases (e.g., Max(x, y, z)).
Nested Max and Min.
Special cases, such as when arguments are identical (e.g., Max(x, x)).
Explicit numerical values.

All the tests passed successfully, which confirms the correctness of the existing implementation.

Would you like me to include these tests in my PR or handle them in a separate one?

oscarbenjamin · 2024-12-01T13:20:47Z

I mean are there any other functions like abs where there is a builtin function and a SymPy function and we would want to be able to do the equivalent of .rewrite(abs) but for a different function.

Ytemiloluwa · 2024-12-02T17:18:04Z

I mean are there any other functions like abs where there is a builtin function and a SymPy function and we would want to be able to do the equivalent of .rewrite(abs) but for a different function.

yes I think the most relevant analogous functions are trigonometric functions (sin/cos) and logarithmic function (log), There might be more

oscarbenjamin · 2024-12-03T00:22:33Z

yes I think the most relevant analogous functions are trigonometric functions (sin/cos) and logarithmic function (log),

None of these are builtins though. I don't think we would want to get into math.sin, cmath.sin, np.sin etc. Maybe abs is the only case.

The reason I am asking about this is because the code change here makes sense if there is exactly one function for which we would want to do this. If there were others then we would probably want some kind of mapping somewhere.

Ytemiloluwa · 2024-12-03T03:39:23Z

yes I think the most relevant analogous functions are trigonometric functions (sin/cos) and logarithmic function (log),

None of these are builtins though. I don't think we would want to get into math.sin, cmath.sin, np.sin etc. Maybe abs is the only case.

The reason I am asking about this is because the code change here makes sense if there is exactly one function for which we would want to do this. If there were others then we would probably want some kind of mapping somewhere.

Agreed, abs appears to be the only case where this specific functionality is relevant.

anutosh491 · 2024-12-03T03:55:00Z

Fom the top of my head ... I can remember python offers these math based intrinsic functions

abs(x)
pow(x, y)
round(x[, ndigits])
divmod(x, y)
max(*args, key=None)
min(*args, key=None)
int(x[, base])
float(x)

So yeah maybe we might have some interest in mapping pow to Pow and abs to Abs but not sure how helpful this would end up to be. Not too many builtins though if that's what concerning in this discussion.

oscarbenjamin · 2024-12-03T11:01:49Z

Probably it is fine just to go with abs then.

An alternative approach could just be to add _eval_rewrite_as_abs to the two classes that support rewriting to Abs:

$ git grep '_eval_rewrite_as_Abs'
sympy/functions/elementary/complexes.py:    def _eval_rewrite_as_Abs(self, arg, **kwargs):
sympy/functions/elementary/miscellaneous.py:    def _eval_rewrite_as_Abs(self, *args, **kwargs):

Ytemiloluwa · 2024-12-03T11:16:48Z

Probably it is fine just to go with abs then.

An alternative approach could just be to add _eval_rewrite_as_abs to the two classes that support rewriting to Abs:
$ git grep '_eval_rewrite_as_Abs'
sympy/functions/elementary/complexes.py:    def _eval_rewrite_as_Abs(self, arg, **kwargs):
sympy/functions/elementary/miscellaneous.py:    def _eval_rewrite_as_Abs(self, *args, **kwargs):

if it is fine to go with abs, will that be all for this PR, or there is a change you want me to incorporate?

anutosh491 · 2024-12-03T11:42:14Z

Hmm, maybe let's stick to what was suggested by Aaron in the issue. He says

So it should be special-cased in rewrite to call Abs.

Which is what @Ytemiloluwa is doing. So lets get this in for now. And maybe address anything that remains if pointed out !

anutosh491 · 2024-12-03T11:44:54Z

Made a small change with respect to the comment (a link as a comment should be enough)

@Ytemiloluwa once your squash the commits into a meaningful one, we can get this in.

Ytemiloluwa · 2024-12-03T11:57:28Z

Made a small change with respect to the comment (a link as a comment should be enough)

@Ytemiloluwa once your squash the commits into a meaningful one, we can get this in.

I have squashed the commits now and all tests have passed @oscarbenjamin @anutosh491

rewrite abs updated test_basic.py updated issue number updated test_rewrite_abs Update test_basic.py

anutosh491 reviewed Nov 29, 2024

View reviewed changes

sympy/core/tests/test_basic.py Outdated Show resolved Hide resolved

anutosh491 reviewed Nov 29, 2024

View reviewed changes

sympy/core/tests/test_basic.py Outdated Show resolved Hide resolved

Ytemiloluwa requested a review from anutosh491 November 29, 2024 13:48

anutosh491 reviewed Nov 29, 2024

View reviewed changes

Ytemiloluwa requested a review from anutosh491 November 29, 2024 15:29

anutosh491 approved these changes Nov 29, 2024

View reviewed changes

StanFromIreland approved these changes Nov 29, 2024

View reviewed changes

Fix: rewrite(abs) to call rewrite(Abs)

604d61a

rewrite abs updated test_basic.py updated issue number updated test_rewrite_abs Update test_basic.py

Ytemiloluwa force-pushed the rewrite_abs branch from e56a4c3 to 604d61a Compare December 3, 2024 12:40

anutosh491 merged commit 2c7f4aa into sympy:master Dec 3, 2024
43 checks passed

oscarbenjamin mentioned this pull request Dec 4, 2024

maint: fix .mailmap file #27348

Merged

anutosh491 mentioned this pull request Dec 5, 2024

Fix rewrite behavior for abs in sign(x) #27327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rewrite(abs) to call rewrite(Abs) #27325

Fix rewrite(abs) to call rewrite(Abs) #27325

Fix rewrite(abs) to call rewrite(Abs) #27325

Fix rewrite(abs) to call rewrite(Abs) #27325

Conversation

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment