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

Comparison of number field elements dependent of real embedding #17830

Closed
videlec opened this issue Feb 22, 2015 · 38 comments
Closed

Comparison of number field elements dependent of real embedding #17830

videlec opened this issue Feb 22, 2015 · 38 comments

Comments

@videlec
Copy link
Contributor

videlec commented Feb 22, 2015

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^3-2, 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:

CC: @sagetrac-tmonteil @jjermann @staroste @gagern @mkoeppe

Component: number fields

Keywords: sd66

Author: Vincent Delecroix, Štěpán Starosta

Branch/Commit: d63b7ac

Reviewer: Volker Braun

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

@videlec videlec added this to the sage-6.6 milestone Feb 22, 2015
@videlec
Copy link
Contributor Author

videlec commented Feb 22, 2015

Commit: 5f7f57b

@videlec
Copy link
Contributor Author

videlec commented Feb 22, 2015

Branch: u/vdelecroix/17830

@videlec
Copy link
Contributor Author

videlec commented Feb 22, 2015

comment:1

Working implementation (i.e. all tests pass in sage.rings.number_fields). Needs some speedup and bullet proofs!


New commits:

1fd916ftrac #17830: ordering for real embedded number fields
316dd8etrac #17830: floor/ceil
5f7f57btrac #17830: cleaner import in number_field.py

@videlec

This comment has been minimized.

@videlec videlec changed the title Comparison of number field elements dependent of embedding Comparison of number field elements dependent of real embedding Feb 22, 2015
@videlec
Copy link
Contributor Author

videlec commented Mar 30, 2015

Changed keywords from none to sd66

@staroste
Copy link
Contributor

staroste commented Apr 1, 2015

Changed commit from 5f7f57b to 82f6d09

@staroste
Copy link
Contributor

staroste commented Apr 1, 2015

Changed branch from u/vdelecroix/17830 to u/sstarosta/17830

@staroste
Copy link
Contributor

staroste commented Apr 1, 2015

comment:5

I have done some testing and committed a few tweaks done on Sage days 66 + doc improvement.

@videlec
Copy link
Contributor Author

videlec commented Apr 1, 2015

comment:6

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

@videlec
Copy link
Contributor Author

videlec commented Apr 1, 2015

Changed branch from u/sstarosta/17830 to u/vdelecroix/17830

@videlec
Copy link
Contributor Author

videlec commented Apr 1, 2015

comment:7

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:

d0f6f7bTrac 17830: simpler initialization + TODO
96ac61eTrac 17830: revert the introduction of _real_mpfi_

@videlec
Copy link
Contributor Author

videlec commented Apr 1, 2015

Changed commit from 82f6d09 to 96ac61e

@videlec
Copy link
Contributor Author

videlec commented Apr 1, 2015

Changed author from Vincent Delecroix to Vincent Delecroix, Štěpán Starosta

@videlec

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2015

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

90b4acctrac #17830: ordering for real embedded number fields
ee5a198trac #17830: floor/ceil
ec50d4dtrac #17830: cleaner import in number_field.py
f07dc08trac #17830: doc improvements and small tweaks
b7ba83aTrac 17830: simpler initialization + TODO

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2015

Changed commit from 96ac61e to b7ba83a

@videlec
Copy link
Contributor Author

videlec commented Apr 29, 2015

comment:10

rebase on sage-6.7.beta3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2015

Changed commit from b7ba83a to a8c294b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2015

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

95f3adatrac #17830: ordering for real embedded number fields
7d0d0c0trac #17830: floor/ceil
2d49517trac #17830: cleaner import in number_field.py
e1efbd6trac #17830: doc improvements and small tweaks
875f756Trac 17830: simpler initialization + TODO
a8c294bTrac #17830: rich_to_bool is no longer a method

@videlec
Copy link
Contributor Author

videlec commented Jun 29, 2015

comment:13

rebased on 6.8.beta6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

Changed commit from a8c294b to eb50b94

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

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

575b264Trac #18159: add a ._test_cardinality in Sets category
cc87bd5Trac #18159: remove a duplicated test
32c403dTrac #18159: fix cardinality/is_finite in sets/set.py
937ebf4Trac #18159: fix some tests related to TestSuite
eb50b94Trac #18159: fix two parents

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

Changed commit from eb50b94 to a8c294b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2015

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

95f3adatrac #17830: ordering for real embedded number fields
7d0d0c0trac #17830: floor/ceil
2d49517trac #17830: cleaner import in number_field.py
e1efbd6trac #17830: doc improvements and small tweaks
875f756Trac 17830: simpler initialization + TODO
a8c294bTrac #17830: rich_to_bool is no longer a method

@videlec
Copy link
Contributor Author

videlec commented Aug 9, 2015

comment:18

Replying to @fchapoton:

+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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2015

Changed commit from a8c294b to 9313849

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2015

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

5b7e757trac #17830: ordering for real embedded number fields
bd7eef0trac #17830: floor/ceil
94b22b6trac #17830: cleaner import in number_field.py
138957atrac #17830: doc improvements and small tweaks
9313849Trac 17830: simpler initialization + TODO

@videlec
Copy link
Contributor Author

videlec commented Dec 7, 2015

New commits:

5b7e757trac #17830: ordering for real embedded number fields
bd7eef0trac #17830: floor/ceil
94b22b6trac #17830: cleaner import in number_field.py
138957atrac #17830: doc improvements and small tweaks
9313849Trac 17830: simpler initialization + TODO

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2015

Changed commit from 9313849 to c7a50e7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2015

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

c7a50e7Trac #17830: add a useless doctest

@vbraun
Copy link
Member

vbraun commented Dec 28, 2015

comment:22

Lgtm but merge conflict...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2015

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

ff13d1ftrac #17830: ordering for real embedded number fields
c52229dtrac #17830: floor/ceil
8543255trac #17830: cleaner import in number_field.py
c098dc7trac #17830: doc improvements and small tweaks
8bb1031Trac #17830: simpler initialization + TODO
d63b7acTrac #17830: add a useless doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2015

Changed commit from c7a50e7 to d63b7ac

@videlec
Copy link
Contributor Author

videlec commented Dec 28, 2015

comment:24

Thanks for having a look! Rebased on 7.0.beta1.

Vincent

@vbraun
Copy link
Member

vbraun commented Dec 28, 2015

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Dec 29, 2015

Changed branch from u/vdelecroix/17830 to d63b7ac

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

4 participants