Opened 2 years ago

Last modified 4 months ago

#27897 needs_work defect

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

Reported by: rburing Owned by:
Priority: major Milestone:
Component: symbolics Keywords: sqrt, abs, complex, hold, holding
Cc: Merged in:
Authors: Dave Morris Reviewers:
Report Upstream: N/A Work issues:
Branch: public/27897 (Commits, GitHub, GitLab) Commit: a3303105570ca4ad4fecb896cb19543029c3da00
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (7)

comment:1 Changed 2 years ago by nbruin

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.

Last edited 2 years ago by nbruin (previous) (diff)

comment:2 Changed 2 years ago by embray

  • Milestone sage-8.8 deleted

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).

comment:3 Changed 4 months ago by gh-DaveWitteMorris

  • Branch set to public/27897

comment:4 Changed 4 months ago by gh-DaveWitteMorris

  • Authors set to Dave Morris
  • Commit set to 357ba2ff7d46ac72cd9ff5968b4755f39554fabf
  • Status changed from new to needs_review

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)

comment:5 Changed 4 months ago by rburing

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.

comment:6 Changed 4 months ago by git

  • Commit changed from 357ba2ff7d46ac72cd9ff5968b4755f39554fabf to a3303105570ca4ad4fecb896cb19543029c3da00

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

a330310review correction

comment:7 Changed 4 months ago by gh-DaveWitteMorris

  • Status changed from needs_review to needs_work

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.

Note: See TracTickets for help on using tickets.