Opened 7 years ago
Closed 6 years ago
#17830 closed enhancement (fixed)
Comparison of number field elements dependent of real embedding
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  number fields  Keywords:  sd66 
Cc:  tmonteil, jj, sstarosta, gagern, mkoeppe  Merged in:  
Authors:  Vincent Delecroix, Štěpán Starosta  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  d63b7ac (Commits, GitHub, GitLab)  Commit:  d63b7ac784f22fa8245694f31c19f3d3d7616e88 
Dependencies:  Stopgaps: 
Description (last modified by )
Comparison of embedded number field elements should be identical to the comparison of the embedding. In other words, we should have:
sage: x = polygen(ZZ) sage: K.<cbrt2> = NumberField(x^3  2, embedding=AA.polynomial_root(x^32, RIF(0,3))) sage: 6064/4813 < cbrt2 < 90325/71691 True
This has been done in #13213 for quadratic number fields. This ticket aims to implement this for all number fields by storing into the parent a list of interval approximations of the generator.
This concerns only real embeddings.
To go further, one should consider:
Change History (26)
comment:1 Changed 7 years ago by
 Branch set to u/vdelecroix/17830
 Commit set to 5f7f57b445adebb118d1d80acb4e930911265494
comment:2 Changed 7 years ago by
 Description modified (diff)
 Summary changed from Comparison of number field elements dependent of embedding to Comparison of number field elements dependent of real embedding
comment:3 Changed 7 years ago by
 Cc sstarosta added
comment:4 Changed 7 years ago by
 Keywords sd66 added
comment:5 Changed 7 years ago by
 Branch changed from u/vdelecroix/17830 to u/sstarosta/17830
 Commit changed from 5f7f57b445adebb118d1d80acb4e930911265494 to 82f6d09a1f62887d0e5b72b81a8def80edb0d414
I have done some testing and committed a few tweaks done on Sage days 66 + doc improvement.
comment:6 Changed 7 years ago by
I am not sure it is because of your commit, but
sage t continued_fraction.py # 1 doctest failed sage t polynomial/polynomial_rational_flint.pyx # 1 doctest failed
comment:7 Changed 7 years ago by
 Branch changed from u/sstarosta/17830 to u/vdelecroix/17830
 Commit changed from 82f6d09a1f62887d0e5b72b81a8def80edb0d414 to 96ac61e74d5dc073fe6d11570ed0be0934c50c77
 Status changed from new to needs_review
The two failing doctests come from the introduction of the method _real_mpfi_
. I just propose to get rid of it in this ticket. The main reason is that there are a lot of duplication between:
 AA and QQbar
 RLF and CLF
 what is implemented in this ticket for number fields
I propose to clean it up in ticket #18103 and add the _real_mpfi_
feature there.
Vincent
New commits:
d0f6f7b  Trac 17830: simpler initialization + TODO

96ac61e  Trac 17830: revert the introduction of _real_mpfi_

comment:8 Changed 7 years ago by
 Cc gagern added
 Description modified (diff)
comment:9 Changed 7 years ago by
 Commit changed from 96ac61e74d5dc073fe6d11570ed0be0934c50c77 to b7ba83a447e1f5cc819b2cb6fa1adfe50fabf826
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
90b4acc  trac #17830: ordering for real embedded number fields

ee5a198  trac #17830: floor/ceil

ec50d4d  trac #17830: cleaner import in number_field.py

f07dc08  trac #17830: doc improvements and small tweaks

b7ba83a  Trac 17830: simpler initialization + TODO

comment:10 Changed 7 years ago by
rebase on sage6.7.beta3
comment:11 Changed 6 years ago by
 Cc mkoeppe added
comment:12 Changed 6 years ago by
 Commit changed from b7ba83a447e1f5cc819b2cb6fa1adfe50fabf826 to a8c294b7188157fb1c168d1eff89ad0c2b200469
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
95f3ada  trac #17830: ordering for real embedded number fields

7d0d0c0  trac #17830: floor/ceil

2d49517  trac #17830: cleaner import in number_field.py

e1efbd6  trac #17830: doc improvements and small tweaks

875f756  Trac 17830: simpler initialization + TODO

a8c294b  Trac #17830: rich_to_bool is no longer a method

comment:13 Changed 6 years ago by
rebased on 6.8.beta6
comment:14 Changed 6 years ago by
 Commit changed from a8c294b7188157fb1c168d1eff89ad0c2b200469 to eb50b94345e732b879ab5faf5c8016252a17d60c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
575b264  Trac #18159: add a ._test_cardinality in Sets category

cc87bd5  Trac #18159: remove a duplicated test

32c403d  Trac #18159: fix cardinality/is_finite in sets/set.py

937ebf4  Trac #18159: fix some tests related to TestSuite

eb50b94  Trac #18159: fix two parents

comment:15 Changed 6 years ago by
 Commit changed from eb50b94345e732b879ab5faf5c8016252a17d60c to a8c294b7188157fb1c168d1eff89ad0c2b200469
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
95f3ada  trac #17830: ordering for real embedded number fields

7d0d0c0  trac #17830: floor/ceil

2d49517  trac #17830: cleaner import in number_field.py

e1efbd6  trac #17830: doc improvements and small tweaks

875f756  Trac 17830: simpler initialization + TODO

a8c294b  Trac #17830: rich_to_bool is no longer a method

comment:16 Changed 6 years ago by
Sorry... I pushed to the wrong ticket. Back to normal.
comment:17 followup: ↓ 18 Changed 6 years ago by
 Status changed from needs_review to needs_work
+Decreased doctests rings/number_field/number_field_base.pyx from 11 / 11 = 100% to 12 / 13 = 92%
comment:18 in reply to: ↑ 17 Changed 6 years ago by
Replying to chapoton:
+Decreased doctests rings/number_field/number_field_base.pyx from 11 / 11 = 100% to 12 / 13 = 92%
Then what? There is no way to test _init_embedding_approx
that initialize private attributes. For sure we can duplicate doctests from _get_embedding_approx
or even write
TESTS:: sage: 1 + 1 # indirect doctest 2
if it makes you happier. But I am not happy with that. If you have any constructive suggestion I would be happy to hear it.
General note: asking for review (= waiting from other people comments) is different from merging into Sage. Patchbot green light concerns the latter.
comment:19 Changed 6 years ago by
 Commit changed from a8c294b7188157fb1c168d1eff89ad0c2b200469 to 9313849ee0acdcb799f3f5f40fc0caa342baddc0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5b7e757  trac #17830: ordering for real embedded number fields

bd7eef0  trac #17830: floor/ceil

94b22b6  trac #17830: cleaner import in number_field.py

138957a  trac #17830: doc improvements and small tweaks

9313849  Trac 17830: simpler initialization + TODO

comment:20 Changed 6 years ago by
 Milestone changed from sage6.6 to sage6.10
 Status changed from needs_work to needs_review
comment:21 Changed 6 years ago by
 Commit changed from 9313849ee0acdcb799f3f5f40fc0caa342baddc0 to c7a50e734790af1f011a5c236542e3b67b2f8aea
Branch pushed to git repo; I updated commit sha1. New commits:
c7a50e7  Trac #17830: add a useless doctest

comment:22 Changed 6 years ago by
 Status changed from needs_review to needs_work
Lgtm but merge conflict...
comment:23 Changed 6 years ago by
 Commit changed from c7a50e734790af1f011a5c236542e3b67b2f8aea to d63b7ac784f22fa8245694f31c19f3d3d7616e88
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ff13d1f  trac #17830: ordering for real embedded number fields

c52229d  trac #17830: floor/ceil

8543255  trac #17830: cleaner import in number_field.py

c098dc7  trac #17830: doc improvements and small tweaks

8bb1031  Trac #17830: simpler initialization + TODO

d63b7ac  Trac #17830: add a useless doctest

comment:24 Changed 6 years ago by
 Status changed from needs_work to needs_review
Thanks for having a look! Rebased on 7.0.beta1.
Vincent
comment:25 Changed 6 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
comment:26 Changed 6 years ago by
 Branch changed from u/vdelecroix/17830 to d63b7ac784f22fa8245694f31c19f3d3d7616e88
 Resolution set to fixed
 Status changed from positive_review to closed
Working implementation (i.e. all tests pass in
sage.rings.number_fields
). Needs some speedup and bullet proofs!New commits:
trac #17830: ordering for real embedded number fields
trac #17830: floor/ceil
trac #17830: cleaner import in number_field.py