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

sqrt(abs(1+I)^2 + 14) evaluates to sqrt(16) instead of 4 #27897

Open
rburing opened this issue May 30, 2019 · 10 comments
Open

sqrt(abs(1+I)^2 + 14) evaluates to sqrt(16) instead of 4 #27897

rburing opened this issue May 30, 2019 · 10 comments

Comments

@rburing
Copy link
Contributor

rburing commented May 30, 2019

There is some weird undesirable "holding" going on:

sage: sqrt(abs(1+I)^2 + 14)
sqrt(16)

Found in Ask SageMath question #46698: Sagemath not evaluating complicated expression.

Component: symbolics

Keywords: sqrt, abs, complex, hold, holding

Author: Dave Morris

Branch/Commit: public/27897 @ a330310

Issue created by migration from https://trac.sagemath.org/ticket/27897

@rburing rburing added this to the sage-8.8 milestone May 30, 2019
@nbruin
Copy link
Contributor

nbruin commented May 30, 2019

comment:1

It looks like there is some double wrapping happening here:

sage: (1+I).pyobject().parent()
Number Field in I with defining polynomial x^2 + 1
sage: abs(1+I).pyobject().parent()
Symbolic Ring
sage: (abs(1+I)^2).pyobject().parent()
Symbolic Ring

here things seem to be still reasonable. However, this last result is "double wrapped":

sage: (abs(1+I)^2).pyobject().pyobject().parent()
Rational Field

and it looks like that's too deep for sqrt to see through. It doesn't happen for this case:

sage: (abs(sqrt(2))^2).pyobject().parent()
Integer Ring

that could be a red herring, though:

sage: b=(1+I)*(1-I)+14
sage: b.pyobject().parent() #this is appropriate
Number Field in I with defining polynomial x^2 + 1
sage: sqrt(b)
sqrt(16)
sage: sqrt(b.pyobject())
4

It means that

sage: a=sqrt(abs((1+I))^2+14)
sage: b=sqrt(norm(1+I)+14)

both lead to unexpectedly "held" expressions, but with different structures:

sage: a.operands()[0].pyobject().parent()
Symbolic Ring
sage: a.operands()[0].pyobject().pyobject().parent()
Rational Field
sage: b.operands()[0].pyobject().parent()
Number Field in I with defining polynomial x^2 + 1

where

sage: b.operands()[0].pyobject().sqrt()
4

so perhaps both problems are solved if sqrt on an SR element would look a little deeper.

@embray
Copy link
Contributor

embray commented Jun 14, 2019

comment:2

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

@embray embray removed this from the sage-8.8 milestone Jun 14, 2019
@DaveWitteMorris
Copy link
Member

Branch: public/27897

@DaveWitteMorris
Copy link
Member

comment:4

I think the problem is gone:

sage: sqrt(abs(1+I)^2 + 14)                                                  
4

This is with 9.3b6 on MacOS 10.15.7, but I get sqrt(16) with 9.2 on CoCalc, so something seems to have changed since then.

The PR adds a doctest. If the patchbots turn green, then I think we can close this ticket.


New commits:

357ba2fdoctest trac 27897 sqrt(16)

@DaveWitteMorris
Copy link
Member

Commit: 357ba2f

@DaveWitteMorris
Copy link
Member

Author: Dave Morris

@rburing
Copy link
Contributor Author

rburing commented Jan 24, 2021

comment:5

I don't have 9.3beta6 here, but I think the definition/parent of I has changed, which may well have hidden this bug instead of fixing it. Could you please check sqrt(abs(1+SR(I))^2 + 14)? The doctest would have to take this form (or similar) as well.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 24, 2021

Changed commit from 357ba2f to a330310

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 24, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

a330310review correction

@DaveWitteMorris
Copy link
Member

comment:7

You're right -- thanks for catching this! With 9.3b6, we still have:

sage: sqrt(abs(1 + SR.I())^2 + 14)
sqrt(16)

Ticket #18036 changed the parent of I to be a number field.

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

No branches or pull requests

4 participants