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

number_field_elements_from_algebraics incorrectly simplify root of unity? #36735

Closed
2 tasks done
user202729 opened this issue Nov 19, 2023 · 6 comments · Fixed by #38606
Closed
2 tasks done

number_field_elements_from_algebraics incorrectly simplify root of unity? #36735

user202729 opened this issue Nov 19, 2023 · 6 comments · Fixed by #38606

Comments

@user202729
Copy link
Contributor

Steps To Reproduce

Run

L, [ww], phi = number_field_elements_from_algebraics([(-1)^(2/3)])
assert ww != 1

Expected Behavior

That ww become a primitive 3rd root of unity.

Actual Behavior

ww = 1.

Additional Information

No response

Environment

- **OS**: Linux
- **Sage Version**: SageMath version 10.1, Release Date: 2023-08-20

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@fchapoton
Copy link
Contributor

Looks like a bug indeed. You could avoid using the symbolic ring:

sage: L, [ww], phi = number_field_elements_from_algebraics([E(3)**2])
sage: L, ww, phi
(Cyclotomic Field of order 3 and degree 2,
 -zeta3 - 1,
 Ring morphism:
   From: Cyclotomic Field of order 3 and degree 2
   To:   Algebraic Field
   Defn: zeta3 |--> -0.500000000000000? + 0.866025403784439?*I)

@DaveWitteMorris
Copy link
Member

I think these are the source of the erroneous answer:

sage: AA((-1)^(2/3))
1
sage: from sage.symbolic.expression_conversions import algebraic
sage: algebraic((-1)^(2/3), AA)
1

Shouldn't they give an error, instead of converting?

@DaveWitteMorris
Copy link
Member

Sorry, I removed the bug tag from the wrong ticket.

@DaveWitteMorris
Copy link
Member

I think this is the problem:

In lines 105-110 of the method arithmetic of class AlgebraicConverter(Converter) (in src/sage/symbolic/expression_conversion/algebraic.py), we see that the expression AA((-1)^(2/3)) is calculated as (AA(-1))**(2/3). But this is not what we want, because the documentation of AA says "AlgebraicReal [is] closed under ... rational powers (including roots), except that for a negative AlgebraicReal, taking a power with an even denominator returns an AlgebraicNumber instead of an AlgebraicReal." This means that raising an element of AA to the 2/3 power will give a result that is in AA. That's not the answer that we want for this expression.

@mezzarobba
Copy link
Member

See also #12074, #12745, #18333.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2024

Looks like a bug indeed. You could avoid using the symbolic ring:

sage: L, [ww], phi = number_field_elements_from_algebraics([E(3)**2])
sage: L, ww, phi
(Cyclotomic Field of order 3 and degree 2,
 -zeta3 - 1,
 Ring morphism:
   From: Cyclotomic Field of order 3 and degree 2
   To:   Algebraic Field
   Defn: zeta3 |--> -0.500000000000000? + 0.866025403784439?*I)

Since

sage: QQbar((-1)^(1/3))
0.500000000000000? + 0.866025403784439?*I
sage: QQbar(E(3))
-0.500000000000000? + 0.866025403784439?*I
sage: QQbar((-1)^(2/3))
-0.500000000000000? + 0.866025403784439?*I
sage: QQbar(E(3)**2)
-0.500000000000000? - 0.866025403784439?*I

E(3)**2 is not equal to (-1)^(2/3) but E(3) is. Note that

sage: L, [ww], phi = number_field_elements_from_algebraics([QQbar((-1)^(2/3))])
sage: L
Cyclotomic Field of order 6 and degree 2
sage: ww
zeta6 - 1
sage: phi
Ring morphism:
  From: Cyclotomic Field of order 6 and degree 2
  To:   Algebraic Field
  Defn: zeta6 |--> 0.500000000000000? + 0.866025403784439?*I

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 27, 2024
sagemathgh-38606: number_field_elements_from_algebraics: Consistently use principal root
    
Fixes sagemath#36735 .

Also this leads to performance improvement because we don't necessarily
waste time convert the elements to algebraic twice.

E.g. if the list of numbers is of the form `[x, y, z]` where `x, y` is a
symbolic expression representing a real algebraic number and `z` is a
symbolic expression representing a complex algebraic number, then we
have to convert `x` and `y` first to `AA` and then to `QQbar`. This is
wasted.

With the new code it's only converted to `QQbar` once. I believe the
conversion from `QQbar` to `AA` should be mostly free?

I think we can all agree that the current behavior is a bug — the odd-
degree root are either interpreted as real root or principal root
depends on whether other complex element appear in the list:

```python
sage: number_field_elements_from_algebraics([(-1)^(2/3), I])  #
interpreted as principal root
(Number Field in a with defining polynomial y^4 - y^2 + 1,
 [a^2 - 1, a^3],
 Ring morphism:
   From: Number Field in a with defining polynomial y^4 - y^2 + 1
   To:   Algebraic Field
   Defn: a |--> 0.866025403784439? + 0.500000000000000?*I)

sage: number_field_elements_from_algebraics([(-1)^(2/3), 2])  #
interpreted as real root
(Rational Field,
 [1, 2],
 Ring morphism:
   From: Rational Field
   To:   Algebraic Real Field
   Defn: 1 |--> 1)
```

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview. (no change)
    
URL: sagemath#38606
Reported by: user202729
Reviewer(s): Kwankyu Lee
@vbraun vbraun closed this as completed in d7ccdb6 Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants