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: Consistently use principal root #38606

Merged
merged 2 commits into from
Sep 29, 2024

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Sep 3, 2024

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

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

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview. (no change)

Copy link

github-actions bot commented Sep 3, 2024

Documentation preview for this PR (built with commit e9c2887; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 26, 2024

@JohnCremona ?

@JohnCremona
Copy link
Member

I have no opinion, positive or negative. I don't think that anyone seriously using number fields or AA or QQbar should ever also be using the symbolic ring, at best it's a shortcut. If I ever find n my own code that something has a value in SR then I regard that as a bug (for me), find out where it happened and fix it so that SR is not used.

I once spent ages helping a colleague try to debug some Mathematica code. It turned out that he had assumed that sqrt(-2)*sqrt(-3)=sqrt(6), but M "assumed" that sqrt(-2)=sqrt(2)*I with sqrt(2)>0, similarly for 3, so sqrt(-2)*sqrt(-3)=-sqrt(6) in that world. There is no way to avoid this sort of thing with "symbolic expressons", they are multi-valued, and no amount of choosing branches will fix all anomalies!

Most of the usees of number_field_elements_from_algebraics() are fine, it's only when some of the numbers passaed are in SR, such as (-1)^(2/3) is. So its the ability of AA and QQbar to create elements out of SR elements which is causing the issue. It might even be worth warning the user whenever that happens, as it can so easily lead to unexpected behaviour, for example QQbar((-1)^(2/3)) and QQbar(AA((-1)^(2/3))) are different.

I have no objection to anyone giving this a positive review.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 26, 2024

Thanks for the comment.

The symbolic expression (-1)^(1/3) or (-1)**(1/3) is very natural code both in the command line and in a script if one wants the principal cube root of -1. The expectation is betrayed because AA(-1)*(1/3) gives -1.

To avoid this anomaly, one is forced to code QQbar((-1)**(1/3)). This is exactly what this PR does.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 26, 2024

This PR strengthens the view that (-1)^(2/3) means the square of the principal cube root -1, and then AA((-1)^(2/3)) giving 1 looks more strange.

Perhaps we cannot escape from all anomalies as John states, but could be more consistent. But this PR is not to solve the inconsistency.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request 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 merged commit d7ccdb6 into sagemath:develop Sep 29, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

number_field_elements_from_algebraics incorrectly simplify root of unity?
5 participants