Opened 7 years ago

Closed 7 years ago

#13765 closed defect (fixed)

Cyclotomic embeddings should respect coercions.

Reported by: robertwb Owned by: davidloeffler
Priority: critical Milestone: sage-5.7
Component: number fields Keywords:
Cc: nbruin Merged in: sage-5.7.beta2
Authors: Robert Bradshaw Reviewers: Frédéric Chapoton, Stephan Ehlen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by chapoton)

sage: zC=(CC(-1)^(1/3))^2
sage: Ka.<aa>=CyclotomicField(3,embedding=zC)
sage: Kb.<bb>=CyclotomicField(3,embedding=zC^2)
sage: Kc.<cc>=CyclotomicField(3,embedding=CC(17))
sage: CC(aa)
-0.500000000000000 + 0.866025403784439*I
sage: CC(bb)
-0.500000000000000 - 0.866025403784439*I
sage: CC(Ka(bb))
-0.500000000000000 + 0.866025403784439*I

It's better to disallow coercions between these then allow this nonsense

sage: CC(aa+bb)
 -1.00000000000000 + 1.73205080756888*I

sage: CC(bb+aa)
 -1.00000000000000 - 1.73205080756888*I

apply trac-13765-cyclo-embeddings.patch

Attachments (1)

trac-13765-cyclo-embeddings.patch (13.3 KB) - added by chapoton 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by robertwb

  • Cc nbruin added
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by robertwb

  • Authors set to Robert Bradshaw
  • Type changed from PLEASE CHANGE to defect

comment:3 Changed 7 years ago by chapoton

One minor comment about the doc :

maybe you could use the trac role :trac:13765 to link to the ticket ?

(see #12490)

Last edited 7 years ago by chapoton (previous) (diff)

comment:4 Changed 7 years ago by chapoton

There is a typo "no such integer exits"

comment:5 Changed 7 years ago by chapoton

I have made the small changes that I suggested :

apply trac-13765-cyclo-embeddings.patch

comment:6 Changed 7 years ago by robertwb

Your changes look fine to me, thanks.

comment:7 Changed 7 years ago by chapoton

  • Description modified (diff)

apply trac-13765-cyclo-embeddings.patch

comment:8 Changed 7 years ago by chapoton

apply trac-13765-cyclo-embeddings.patch

comment:9 Changed 7 years ago by ehlen

  • Status changed from needs_review to needs_work
  • I tested the patch using Sage 5.5.
  • It is well documented and it solves the bug as far as I can see.
  • The documentation builds.

But I am not convinced that the implementation of NumberField_cyclotomic._log_gen should stay like it is. It is a private method but it might be used within the class for ther purposes later. Therefore, I think the output should always make sense. But it doesn't always and it is not completely consistent. Here is an Example.

This is what I expect from the documentation:

 sage: K.<zeta>=CyclotomicField(5, embedding=CC(exp(2*pi*i/5)))    
 sage: K._log_gen(10)
 sage: type(K._log_gen(10))
 <type 'NoneType'>

So, this is o.k., but:

 sage: K.<zeta>=CyclotomicField(5, embedding=CC(exp(2*pi*i/5)))    
 sage: K._log_gen(CDF(10))
 sage: 0
 sage: type(K._log_gen(CDF(10)))
 sage: <type 'sage.rings.integer.Integer'>

This behaviour is more or less documented in the docstring. It says that "If x is complex, returns the nearest such e whether or not the result is exact.". But I don't agree that it should be this way.

But I think this is not consistent and it should be fixed.

Version 0, edited 7 years ago by ehlen (next)

Changed 7 years ago by chapoton

comment:10 Changed 7 years ago by chapoton

Here is a better patch, as we discussed

apply trac-13765-cyclo-embeddings.patch

comment:11 Changed 7 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:12 Changed 7 years ago by robertwb

As you over-write the previous patch, I can't tell what changed. Is it just the two-line sanity check and corresponding change to the documentation? Seems like a OK restriction to me, in which case positive review to your changes.

comment:13 Changed 7 years ago by chapoton

  • Reviewers set to Frédéric Chapoton, Stephan Ehlen
  • Status changed from needs_review to positive_review

comment:14 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.7.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.