Opened 8 years ago
Closed 8 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 )
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)
Change History (15)
comment:1 Changed 8 years ago by
- Cc nbruin added
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Type changed from PLEASE CHANGE to defect
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
There is a typo "no such integer exits"
comment:5 Changed 8 years ago by
I have made the small changes that I suggested :
apply trac-13765-cyclo-embeddings.patch
comment:6 Changed 8 years ago by
Your changes look fine to me, thanks.
comment:7 Changed 8 years ago by
- Description modified (diff)
apply trac-13765-cyclo-embeddings.patch
comment:8 Changed 8 years ago by
apply trac-13765-cyclo-embeddings.patch
comment:9 Changed 8 years ago by
- 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.
Changed 8 years ago by
comment:10 Changed 8 years ago by
Here is a better patch, as we discussed
apply trac-13765-cyclo-embeddings.patch
comment:11 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:12 Changed 8 years ago by
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 8 years ago by
- Reviewers set to Frédéric Chapoton, Stephan Ehlen
- Status changed from needs_review to positive_review
comment:14 Changed 8 years ago by
- Merged in set to sage-5.7.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
One minor comment about the doc :
maybe you could use the trac role :trac:
13765
to link to the ticket ?(see #12490)