Opened 2 years ago

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

Reported by: Owned by: rburing major symbolics sqrt, abs, complex, hold, holding Dave Morris N/A public/27897 a3303105570ca4ad4fecb896cb19543029c3da00

### Description

There is some weird undesirable "holding" going on:

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

### 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:

 ​357ba2f `doctest 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:

 ​a330310 `review 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.