Opened 6 months ago

Closed 5 months ago

#31628 closed defect (fixed)

fix/improve conversions to QQbar and AA

Reported by: mmezzarobba Owned by:
Priority: blocker Milestone: sage-9.3
Component: coercion Keywords:
Cc: Merged in:
Authors: Marc Mezzarobba, Vincent Delecroix Reviewers: Vincent Delecroix, Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: ed19558 (Commits, GitHub, GitLab) Commit: ed195584c70c28629ea7ab83557d6fe2877c1f3d
Dependencies: Stopgaps:

Status badges

Description

  • fix conversion from ℚ[i] to QQbar (honor complex embeddings)
  • add implicit coercions from python ints and from number fields with compatible embeddings

Change History (15)

comment:1 Changed 6 months ago by mmezzarobba

  • Branch set to u/mmezzarobba/31628-toqqbar
  • Commit set to cf414b6a4e9bd6b56d9a017b5de5af21f0aea561
  • Status changed from new to needs_review

New commits:

​43511e4fix conversion from QQ[-i] to QQbar
​cf414b6new implicit coercions to QQbar and AA

comment:2 Changed 6 months ago by slelievre

Not sure about that kind of humour: 

            sage: NF.<sqrt3> = QuadraticField(2)
            sage: AA(sqrt3)
            1.414213562373095?

comment:3 Changed 6 months ago by git

  • Commit changed from cf414b6a4e9bd6b56d9a017b5de5af21f0aea561 to cf5228bf29d5f3e5bd913c23feec854cdb09abd1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​cf5228bnew implicit coercions to QQbar and AA

comment:4 Changed 6 months ago by mmezzarobba

fixed, thanks

comment:5 Changed 6 months ago by vdelecroix

Conversions always existed I think. Could you doctest more directly what changed, namely 

sage: QQbar.has_coerce_map_from(QuadraticField(-1))

and 

sage: K = NumberField(x^3 - 2, 'a', embedding=2.**(1/3))
sage: AA.has_coerce_map_from(K)

I will adapt #30518.

comment:6 Changed 6 months ago by git

  • Commit changed from cf5228bf29d5f3e5bd913c23feec854cdb09abd1 to 6430356bdd45a63ebc847ca1284aac7abaf441d7

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

​6430356#31628 one more test

comment:7 Changed 6 months ago by mmezzarobba

The first case is already tested by 

            sage: i + QQbar(2)
            I + 2
            sage: K.<ii> = QuadraticField(-1, embedding=ComplexField(13)(0,-1))
            sage: ii + QQbar(2)
            -I + 2

(better IMO, since it checks that the embedding is respected).

I added the second one so that we have an example or degree >2.

New commits:

​6430356#31628 one more test

comment:8 Changed 6 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

Parfait. Merci.

comment:9 Changed 5 months ago by mkoeppe

  • Priority changed from major to blocker

comment:10 Changed 5 months ago by vbraun

  • Branch changed from u/mmezzarobba/31628-toqqbar to 6430356bdd45a63ebc847ca1284aac7abaf441d7
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:11 Changed 5 months ago by vbraun

  • Commit 6430356bdd45a63ebc847ca1284aac7abaf441d7 deleted
  • Resolution fixed deleted
  • Status changed from closed to new 
sage -t --long --warn-long 45.5 --random-seed=0 src/sage/symbolic/expression.pyx  # 1 doctest failed
sage -t --long --warn-long 45.5 --random-seed=0 src/sage/modular/dirichlet.py  # 1 doctest failed

comment:12 Changed 5 months ago by vdelecroix

Indeed 

File "symbolic/expression.pyx", line 3155, in sage.symbolic.expression.Expression.__nonzero__
Failed example:
    bool(SR(QQbar(I)) == I)
Expected:
    Traceback (most recent call last):
    ...
    TypeError: unsupported operand parent(s)...
Got:
    True

and 

sage -t --long --warn-long 45.5 --random-seed=0 modular/dirichlet.py
**********************************************************************
File "modular/dirichlet.py", line 1593, in sage.modular.dirichlet.DirichletCharacter.kloosterman_sum
Failed example:
    e.kloosterman_sum(5,11)
Expected:
    Traceback (most recent call last):
    ...
    NotImplementedError: Kloosterman sums not implemented over this ring
Got:
    0

comment:13 Changed 5 months ago by vdelecroix

  • Branch changed from 6430356bdd45a63ebc847ca1284aac7abaf441d7 to public/31628
  • Commit set to ed195584c70c28629ea7ab83557d6fe2877c1f3d
  • Status changed from new to needs_review

New commits:

​43511e4fix conversion from QQ[-i] to QQbar
​cf5228bnew implicit coercions to QQbar and AA
​6430356#31628 one more test
​ed19558#31628: two doctests

comment:14 Changed 5 months ago by mmezzarobba

  • Authors changed from Marc Mezzarobba to Marc Mezzarobba, Vincent Delecroix
  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Marc Mezzarobba
  • Status changed from needs_review to positive_review

Thank you Volker and Vincent!

comment:15 Changed 5 months ago by vbraun

  • Branch changed from public/31628 to ed195584c70c28629ea7ab83557d6fe2877c1f3d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.