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

Coercion between quadratic fields fails #28774

Closed
kliem opened this issue Nov 20, 2019 · 27 comments
Closed

Coercion between quadratic fields fails #28774

kliem opened this issue Nov 20, 2019 · 27 comments

Comments

@kliem
Copy link
Contributor

kliem commented Nov 20, 2019

sage: K.<sqrt2> = QuadraticField(2)
sage: K.<sqrt3> = QuadraticField(3)
sage: sqrt2 + sqrt3
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-5ef32d62030c> in <module>()
----> 1 sqrt2 + sqrt3

/srv/public/shared/sage-8.9/local/lib/python2.7/site-packages/sage/structure/element.pyx in sage.structure.element.Element.__add__ (build/cythonized/sage/structure/element.c:10816)()
   1234         # Left and right are Sage elements => use coercion model
   1235         if BOTH_ARE_ELEMENT(cl):
-> 1236             return coercion_model.bin_op(left, right, add)
   1237 
   1238         cdef long value

/srv/public/shared/sage-8.9/local/lib/python2.7/site-packages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:10895)()
   1205         # We should really include the underlying error.
   1206         # This causes so much headache.
-> 1207         raise bin_op_exception(op, x, y)
   1208 
   1209     cpdef canonical_coercion(self, x, y):

TypeError: unsupported operand parent(s) for +: 'Number Field in sqrt2 with defining polynomial x^2 - 2 with sqrt2 = 1.414213562373095?' and 'Number Field in sqrt3 with defining polynomial x^2 - 3 with sqrt3 = 1.732050807568878?'

This is by design, as it is hard to guess the users intentions:

  • Exact computations should only be done in AA for exploratory work, as it is precise but possibly very slow. Exact conversion to AA works:
sage: K.<sqrt2> = QuadraticField(2)
sage: AA(sqrt2)
1.414213562373095?
sage: AA(sqrt2) == AA(2).sqrt()
True
  • Otherwise, exact computations should be done in a good common number field, e.g. using composite_fields:
sage: K2.<sqrt2> = QuadraticField(2)
sage: K3.<sqrt3> = QuadraticField(3)
sage: K = K2.composite_fields(K3)[0]
sage: K(sqrt2)
-1/2*sqrt2sqrt3^3 + 9/2*sqrt2sqrt3
sage: K(sqrt3)
-1/2*sqrt2sqrt3^3 + 11/2*sqrt2sqrt3
sage: K
Number Field in sqrt2sqrt3 with defining polynomial x^4 - 10*x^2 + 1 with sqrt2sqrt3 = 0.3178372451957823?

However, there is a choice involved in picking the common number field, which is up to the user.

  • Also there is reliable fast but imprecise float calculations. Number fields can be coerced into the reals, but this wouldn't be a good choice for a canonical common parent of two number fields.

Depends on #28778

Component: number fields

Keywords: quadratic field, embeddings

Reviewer: Sébastien Labbé

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

@kliem kliem added this to the sage-9.0 milestone Nov 20, 2019
@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

New commits:

af2d160embed quadratic field into algebraic (real) numbers

@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

Commit: af2d160

@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

Branch: public/28774

@kliem
Copy link
Contributor Author

kliem commented Nov 23, 2019

comment:3

Many failing tests and various places.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2019

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

d20ed43fixed some failing tests
a4a7348fixing some tests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2019

Changed commit from af2d160 to a4a7348

@kliem
Copy link
Contributor Author

kliem commented Nov 25, 2019

Changed author from Jonathan Kliem to none

@kliem

This comment has been minimized.

@kliem kliem changed the title Embed QuadraticField into algebraic numbers by default Coercion between quadratic fields fails Nov 25, 2019
@vbraun
Copy link
Member

vbraun commented Nov 29, 2019

comment:6

This seems like it would be a good idea to post to sage-devel if only just to understand the rationale of the current state.

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Dec 5, 2019

Changed branch from public/28774 to none

@kliem
Copy link
Contributor Author

kliem commented Dec 5, 2019

Changed commit from a4a7348 to none

@kliem kliem removed this from the sage-9.0 milestone Dec 5, 2019
@embray
Copy link
Contributor

embray commented Dec 6, 2019

comment:8

Perhaps a more useful error message could be given here? The hint about using composite fields seems especially helpful.

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

comment:9

Yes, I was thinking about this as well. Wasn't sure where to put this.

I could give such a message in the _pushout_ method of number fields. This method will be called for sure and I think its also the last hope for coercion to work in case of number fields.

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Dependencies: #28778

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Branch: public/28774-new

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Commit: ad8d632

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

comment:11

This is the best I can come up with.

To my understanding coercion of two number fields is only possible, if they are both embedded into AA (or QQbar with #28778) or if there is an embedding (discovered by _internal_coerce_map_from).

Raising an error or a warning does not work, as it is being suppressed.

This depends on #28778 now, as this fixes an error (along the way), where the pushout of two number fields embedded into RLF is AA. This ticket relies on the method _pushout_ to return None in this case.


New commits:

b4b2cf1pushout for number fields with compatible embeddings
d46d6bbremove unneeded test
9a1bce3small mistake in docstests
8e70251defining the pushout for number fields just for AA and QQbar
0d1fc2dMerge branch 'public/28778-bis' of git://trac.sagemath.org/sage into public/28774-new
ad8d632printed a message pointing towards composite fields

@kliem kliem added this to the sage-9.0 milestone Dec 6, 2019
@embray
Copy link
Contributor

embray commented Jan 6, 2020

comment:12

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-9.0, sage-9.1 Jan 6, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 16, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 14, 2020

Author: Jonathan Kliem

@seblabbe
Copy link
Contributor

comment:15

Patchbot says:

----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/schemes/elliptic_curves/ell_number_field.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/schemes/elliptic_curves/gal_reps_number_field.py  # 4 doctests failed
sage -t --long --random-seed=0 src/sage/categories/pushout.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/libs/flint/nmod_poly_linkage.pxi  # Timed out (and interrupt failed)
----------------------------------------------------------------------

in particular:

sage -t --long --random-seed=0 src/sage/categories/pushout.py
**********************************************************************
File "src/sage/categories/pushout.py", line 3164, in sage.categories.pushout.AlgebraicExtensionFunctor.merge
Failed example:
    c1+c2; parent(c1+c2)    #indirect doctest
Expected:
    -b^6 + b^4 - 1
    Number Field in b with defining polynomial x^8 - x^4 + 1 with b = -0.2588190451025208? + 0.9659258262890683?*I
Got:
    common parent not found; the method ``composite_fields`` might be useful to find a common parent for number fields
    common parent not found; the method ``composite_fields`` might be useful to find a common parent for number fields
    -b^6 + b^4 - 1
    Number Field in b with defining polynomial x^8 - x^4 + 1 with b = -0.2588190451025208? + 0.9659258262890683?*I
**********************************************************************
1 item had failures:
   1 of  24 in sage.categories.pushout.AlgebraicExtensionFunctor.merge
    [876 tests, 1 failure, 12.84 s]

and

sage -t --long --random-seed=0 src/sage/schemes/elliptic_curves/ell_number_field.py
**********************************************************************
File "src/sage/schemes/elliptic_curves/ell_number_field.py", line 3303, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.?
Failed example:
    rho.reducible_primes()
Expected:
    [2, 3]
Got:
    common parent not found; the method ``composite_fields`` might be useful to find a common parent for number fields
    common parent not found; the method ``composite_fields`` might be useful to find a common parent for number fields
    [2, 3]
**********************************************************************
1 item had failures:
   1 of  84 in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.?
    [814 tests, 1 failure, 176.43 s]

and the patchbot also says pyflakes warnings:

========== pyflakes ==========
git checkout patchbot/ticket_merged
Already on 'patchbot/ticket_merged'
src/sage/rings/number_field/number_field.py:9708:42 '...' % ... has 1 placeholder(s) but 2 substitution(s)

found 1 pyflakes errors in the modified files
Traceback (most recent call last):
  File "/home/sagemath/.local/lib/python3.7/site-packages/sage_patchbot/patchbot.py", line 1109, in test_a_ticket
    baseline=baseline, **kwds)
  File "/home/sagemath/.local/lib/python3.7/site-packages/sage_patchbot/plugins.py", line 281, in pyflakes
    raise ValueError(full_msg)
ValueError: found 1 pyflakes errors in the modified files
pyflakes -- 0 seconds
========== end pyflakes ==========

and pycodestyle warnings:

========== pycodestyle ==========
git checkout patchbot/ticket_merged
Already on 'patchbot/ticket_merged'
src/sage/rings/number_field/number_field.py:5038:34: E702 multiple statements on one line (semicolon)
src/sage/rings/number_field/number_field.py:5061:25: E702 multiple statements on one line (semicolon)
src/sage/rings/number_field/number_field.py:5062:25: E702 multiple statements on one line (semicolon)
found 3 invalid escape sequences in the modified files
Traceback (most recent call last):
  File "/home/sagemath/.local/lib/python3.7/site-packages/sage_patchbot/patchbot.py", line 1109, in test_a_ticket
    baseline=baseline, **kwds)
  File "/home/sagemath/.local/lib/python3.7/site-packages/sage_patchbot/plugins.py", line 344, in pycodestyle
    raise ValueError(full_msg)
ValueError: found 3 invalid escape sequences in the modified files
pycodestyle -- 0 seconds
========== end pycodestyle ==========

@kliem
Copy link
Contributor Author

kliem commented Aug 16, 2020

comment:16

I posted about this on sage-devel https://groups.google.com/d/msg/sage-devel/oSFceE75pWU/K-akRxS6BAAJ.

At the moment it looks like there isn't much interest. I'll wait a day and if nothing shows up, we can just close this here as invalid.

@kliem
Copy link
Contributor Author

kliem commented Aug 17, 2020

Changed author from Jonathan Kliem to none

@kliem
Copy link
Contributor Author

kliem commented Aug 17, 2020

Changed commit from ad8d632 to none

@kliem
Copy link
Contributor Author

kliem commented Aug 17, 2020

Changed branch from public/28774-new to none

@kliem kliem removed this from the sage-9.2 milestone Aug 17, 2020
@seblabbe
Copy link
Contributor

comment:18

ok

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 19, 2020

Reviewer: Sébastien Labbé

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

6 participants