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

QQ.number_field() does not behave like any other NumberField #7596

Open
rlmill mannequin opened this issue Dec 3, 2009 · 12 comments
Open

QQ.number_field() does not behave like any other NumberField #7596

rlmill mannequin opened this issue Dec 3, 2009 · 12 comments

Comments

@rlmill
Copy link
Mannequin

rlmill mannequin commented Dec 3, 2009

Several examples:

sage: K.<a> = NumberField(x)
sage: K.ideal(5)
Fractional ideal (5)
sage: QQ.ideal(5)
Principal ideal (1) of Rational Field
sage: QQ.number_field().ideal(5)
Principal ideal (1) of Rational Field
K = QQ
I = K.ideal(7)

This creates ideal that does not have the functions I.denominator, I.numerator, I.prime_ideals() ... which a fractional ideal in a number field should have

K.<a> = NumberField(x^2+2)
I = K.ideal(7)

Similarly, QQ.places() is not implemented; it should return the one infinite place for Q. Although there seems to be QQ.embeddings().

QQ.places()

CC: @slel

Component: number fields

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

@rlmill rlmill mannequin added this to the sage-4.6.2 milestone Dec 3, 2009
@rlmill rlmill mannequin added c: number fields labels Dec 3, 2009
@rlmill rlmill mannequin assigned loefflerd Dec 3, 2009
@koffie koffie modified the milestone: sage-4.6.2 Feb 10, 2011
@koffie
Copy link

koffie commented Feb 10, 2011

comment:3

sorry modified the wrong ticket, i meant to edit #9414 which is a duplicate of this one

@jdemeyer

This comment has been minimized.

@fchapoton
Copy link
Contributor

Attachment: trac_7596_places_for_QQ.patch.gz

@fchapoton
Copy link
Contributor

comment:5

here is already a ticket for the easy part : a method "places" for QQ

@fchapoton
Copy link
Contributor

comment:6

I have imported a patch trying to have QQ and number fields in the same categories:

sage: K.<phi> = NumberField(x**2-x-1)
sage: K.categories() == QQ.categories()
True

but this breaks the lcm and gcd in a bad way..

@fchapoton
Copy link
Contributor

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@JohnCremona
Copy link
Member

comment:11

As of 9.3.beta3, certainly QQ.places() does work. What I usually do is, when I find a method which QQ does not have but a general number field does have, in the course of implementing something, I just add an aimplementation for QQ. I think this is more efficient than trying to make QQ work like a number field. There will be a large number of users who get to see QQ who never use more general number fields. For such people it does not matter at all to have methods like QQ.places() whose use they may not know. There were many arguments about what QQ.ideal(5) should return, since the pedantic aswer (as for any field, with a nonzero generator) is that it has to be "Principal ideal (1) of ...". Number theorists are quite happy to have K.ideal(...) meaning K.fractional_ideal(...).

Is there any reason not to mark this as invalid/won't fix?

@JohnCremona JohnCremona removed this from the sage-6.4 milestone Feb 9, 2021
@DaveWitteMorris
Copy link
Member

comment:13

-1 for closing. I think comment:11 discloses a serious problem, though not exactly what is emphasized in the ticket description. Number fields are rings:

sage: K.<a> = NumberField(x)
sage: isinstance(K, Ring)
True

Thus, the ideal method of a number field needs to return an ideal of self (not an ideal of some subring of self), because that is what is expected by the ideal method of a Ring. Otherwise, well-written code involving rings will be buggy.

I think that if number theorists are not willing to accept this, then NumberField should not inherit from Ring. But I think it would be better to introduce a new method (maybe integer_ideal) to the NumberField class, or add a keyword flag to the ideal method.

@JohnCremona
Copy link
Member

comment:14

We have had this discussion many times over the years. I think I first had it in Jan 2008. There are correct term for these: "integral ideals" and "fractional ideals". Calling them "ideals" is an abuse of language (agreed), just a common one. There is no reason at all not to use "fractional_ideal" as the method name -- the class name is already NumberFieldFractionalIdeal. But it would break a lot of users' code (including mine) so it is not a high priority for me.

This ticket has been open for 11 years...

@nbruin
Copy link
Contributor

nbruin commented Aug 2, 2021

comment:15

I think I can live with the practical effect of the implementation of is_prime on number field elements, but note that its documentation says:

        Is ``self`` a prime element?

        A *prime* element is a non-zero, non-unit element `p` such that,
        whenever `p` divides `ab` for some `a` and `b`, then `p`
        divides `a` or `p` divides `b`.
...
        In fields, an element is never prime::
...

so its documentation directly contradicts its behaviour. This is due to the fact that the method is inherited from Ring, but the ideal protocol is not compatibly implemented. That obviously needs fixing. In this case, it probably means equipping number field elements with a fresh implementation of is_prime with appropriate doc.

Reassigning this ticket to a milestone, because having documentation directly contradict behaviour is obviously something that not just generic "low priority" or "not a bug".

@nbruin nbruin added this to the sage-9.5 milestone Aug 2, 2021
@yyyyx4
Copy link
Member

yyyyx4 commented Aug 12, 2021

comment:16

Replying to @nbruin:

In this case, it probably means equipping number field elements with a fresh implementation of is_prime with appropriate doc.

I had a shot at this over at #32340.

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 18, 2021

comment:17

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 11, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 29, 2024
… `.discriminant()` and `.is_isomorphic` of quaternion algebras

    
1. Removed unnecessary product and factorization for
`.ramified_primes()`
2. Adapted `is_isomorphic` to reduce unnecessary calculations
3. Fixed a bug in `.discriminant()` and `.ramified_primes()` where
rational invariants caused errors
4. Removed `.hilbert_conductor` from `sage.arith.misc` import list and
added `.hilbert_symbol`
5. Reduced restriction of `.ramified_primes()` to rational quaternion
algebras in docstring in preparation for planned `.ramified_places()`
function over number fields (currently being worked on)

In more detail:
1. The original workflow for `.ramified_primes()` went as follows:
    - Call `.discriminant()`, which calls `.hilbert_conductor`
    - Inside `.hilbert_conductor`, the ramified primes are computed and
their product (the discriminant of the quaternion algebra) is returned
    - Finally, factor the discriminant back into its prime factors

    Hence we have a redundant product and, more crucially, a redundant
prime factorization. This fix modifies `.ramified_primes()` to instead
directly build the list computed in `.hilbert_conductor()` (up to a bug
fix described in 3.) and return it; the list might not always be sorted
by magnitude of primes, so an optional argument `sorted` (set to `False`
by default) allows to enforce this (small to large) sorting.
Furthermore, `.discriminant()` has been adapted to directly take the
product of the list returned by `.ramified_primes()` (only in the
rational case, for now - see 5.)

2. Since the `.discriminant()`-function needs to compute all (finite)
ramified primes (this was also true before this PR, it was just hidden
inside `.hilbert_conductor()` instead), the function `.is_isomorphic()`
now compares the unsorted lists of finite ramified primes to decide
whether two rational quaternion algebras are isomorphic.

3. The function `sage.arith.misc.hilbert_conductor` requires its
arguments to be integers (to create certain lists of prime divisors);
since it was originally used to determine the discriminant (and, as
explained in 1., the ramified primes), it raises an error when the
invariants are proper rational numbers. To get around the analogous
error for the method `.hilbert_symbol`, we instead look at the
numerators and denominators of both invariants separately, using the
fact that we can (purely on a mathematical level) rescale both
invariants by the squares of their respective denominators without
leaving the isomorphism class of the algebra.

4. The only call to `sage.arith.misc.hilbert_conductor` in
quaternion_algebra.py was given in the old computation of the
discriminant (the other `.hilbert_conductor` in the code, also in
`.discriminant()`, refers to the one in `sage.rings.number_field`), so
it was removed from the import list. The new approach to
`.ramified_primes()` requires `sage.arith.misc.hilbert_symbol`, which
was added to the import list.

5. As of now the `.ramified_primes()`-method is only supported for
rational quaternion algebras. I'm currently working on a version over
number fields, but once it works correctly this will be implemented as a
new function `.ramified_places` (Update: see sagemath#37173) ~~to distinguish
between different formats (prime numbers vs ideals) over $\mathbb{Q}$~~
(Update: this wasn't really feasible, see the issues discussed in sagemath#7596;
thanks to @yyyyx4 for pointing me towards this discussion) ~~and,
furthermore,~~ to not cause confusion using the term "primes" for the
Archimedean real places where a quaternion algebra might ramify. Hence
the implementation restriction in the docstring of `.ramified_primes()`
was removed, but the method still throws a ValueError if not called with
a quaternion algebra defined over the rational numbers.

#sd123
    
URL: sagemath#37164
Reported by: Sebastian Spindler
Reviewer(s): grhkm21, Sebastian Spindler
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 30, 2024
… `.discriminant()` and `.is_isomorphic` of quaternion algebras

    
1. Removed unnecessary product and factorization for
`.ramified_primes()`
2. Adapted `is_isomorphic` to reduce unnecessary calculations
3. Fixed a bug in `.discriminant()` and `.ramified_primes()` where
rational invariants caused errors
4. Removed `.hilbert_conductor` from `sage.arith.misc` import list and
added `.hilbert_symbol`
5. Reduced restriction of `.ramified_primes()` to rational quaternion
algebras in docstring in preparation for planned `.ramified_places()`
function over number fields (currently being worked on)

In more detail:
1. The original workflow for `.ramified_primes()` went as follows:
    - Call `.discriminant()`, which calls `.hilbert_conductor`
    - Inside `.hilbert_conductor`, the ramified primes are computed and
their product (the discriminant of the quaternion algebra) is returned
    - Finally, factor the discriminant back into its prime factors

    Hence we have a redundant product and, more crucially, a redundant
prime factorization. This fix modifies `.ramified_primes()` to instead
directly build the list computed in `.hilbert_conductor()` (up to a bug
fix described in 3.) and return it; the list might not always be sorted
by magnitude of primes, so an optional argument `sorted` (set to `False`
by default) allows to enforce this (small to large) sorting.
Furthermore, `.discriminant()` has been adapted to directly take the
product of the list returned by `.ramified_primes()` (only in the
rational case, for now - see 5.)

2. Since the `.discriminant()`-function needs to compute all (finite)
ramified primes (this was also true before this PR, it was just hidden
inside `.hilbert_conductor()` instead), the function `.is_isomorphic()`
now compares the unsorted lists of finite ramified primes to decide
whether two rational quaternion algebras are isomorphic.

3. The function `sage.arith.misc.hilbert_conductor` requires its
arguments to be integers (to create certain lists of prime divisors);
since it was originally used to determine the discriminant (and, as
explained in 1., the ramified primes), it raises an error when the
invariants are proper rational numbers. To get around the analogous
error for the method `.hilbert_symbol`, we instead look at the
numerators and denominators of both invariants separately, using the
fact that we can (purely on a mathematical level) rescale both
invariants by the squares of their respective denominators without
leaving the isomorphism class of the algebra.

4. The only call to `sage.arith.misc.hilbert_conductor` in
quaternion_algebra.py was given in the old computation of the
discriminant (the other `.hilbert_conductor` in the code, also in
`.discriminant()`, refers to the one in `sage.rings.number_field`), so
it was removed from the import list. The new approach to
`.ramified_primes()` requires `sage.arith.misc.hilbert_symbol`, which
was added to the import list.

5. As of now the `.ramified_primes()`-method is only supported for
rational quaternion algebras. I'm currently working on a version over
number fields, but once it works correctly this will be implemented as a
new function `.ramified_places` (Update: see sagemath#37173) ~~to distinguish
between different formats (prime numbers vs ideals) over $\mathbb{Q}$~~
(Update: this wasn't really feasible, see the issues discussed in sagemath#7596;
thanks to @yyyyx4 for pointing me towards this discussion) ~~and,
furthermore,~~ to not cause confusion using the term "primes" for the
Archimedean real places where a quaternion algebra might ramify. Hence
the implementation restriction in the docstring of `.ramified_primes()`
was removed, but the method still throws a ValueError if not called with
a quaternion algebra defined over the rational numbers.

#sd123
    
URL: sagemath#37164
Reported by: Sebastian Spindler
Reviewer(s): grhkm21, Sebastian Spindler
vbraun pushed a commit to vbraun/sage that referenced this issue Feb 1, 2024
… `.discriminant()` and `.is_isomorphic` of quaternion algebras

    
1. Removed unnecessary product and factorization for
`.ramified_primes()`
2. Adapted `is_isomorphic` to reduce unnecessary calculations
3. Fixed a bug in `.discriminant()` and `.ramified_primes()` where
rational invariants caused errors
4. Removed `.hilbert_conductor` from `sage.arith.misc` import list and
added `.hilbert_symbol`
5. Reduced restriction of `.ramified_primes()` to rational quaternion
algebras in docstring in preparation for planned `.ramified_places()`
function over number fields (currently being worked on)

In more detail:
1. The original workflow for `.ramified_primes()` went as follows:
    - Call `.discriminant()`, which calls `.hilbert_conductor`
    - Inside `.hilbert_conductor`, the ramified primes are computed and
their product (the discriminant of the quaternion algebra) is returned
    - Finally, factor the discriminant back into its prime factors

    Hence we have a redundant product and, more crucially, a redundant
prime factorization. This fix modifies `.ramified_primes()` to instead
directly build the list computed in `.hilbert_conductor()` (up to a bug
fix described in 3.) and return it; the list might not always be sorted
by magnitude of primes, so an optional argument `sorted` (set to `False`
by default) allows to enforce this (small to large) sorting.
Furthermore, `.discriminant()` has been adapted to directly take the
product of the list returned by `.ramified_primes()` (only in the
rational case, for now - see 5.)

2. Since the `.discriminant()`-function needs to compute all (finite)
ramified primes (this was also true before this PR, it was just hidden
inside `.hilbert_conductor()` instead), the function `.is_isomorphic()`
now compares the unsorted lists of finite ramified primes to decide
whether two rational quaternion algebras are isomorphic.

3. The function `sage.arith.misc.hilbert_conductor` requires its
arguments to be integers (to create certain lists of prime divisors);
since it was originally used to determine the discriminant (and, as
explained in 1., the ramified primes), it raises an error when the
invariants are proper rational numbers. To get around the analogous
error for the method `.hilbert_symbol`, we instead look at the
numerators and denominators of both invariants separately, using the
fact that we can (purely on a mathematical level) rescale both
invariants by the squares of their respective denominators without
leaving the isomorphism class of the algebra.

4. The only call to `sage.arith.misc.hilbert_conductor` in
quaternion_algebra.py was given in the old computation of the
discriminant (the other `.hilbert_conductor` in the code, also in
`.discriminant()`, refers to the one in `sage.rings.number_field`), so
it was removed from the import list. The new approach to
`.ramified_primes()` requires `sage.arith.misc.hilbert_symbol`, which
was added to the import list.

5. As of now the `.ramified_primes()`-method is only supported for
rational quaternion algebras. I'm currently working on a version over
number fields, but once it works correctly this will be implemented as a
new function `.ramified_places` (Update: see sagemath#37173) ~~to distinguish
between different formats (prime numbers vs ideals) over $\mathbb{Q}$~~
(Update: this wasn't really feasible, see the issues discussed in sagemath#7596;
thanks to @yyyyx4 for pointing me towards this discussion) ~~and,
furthermore,~~ to not cause confusion using the term "primes" for the
Archimedean real places where a quaternion algebra might ramify. Hence
the implementation restriction in the docstring of `.ramified_primes()`
was removed, but the method still throws a ValueError if not called with
a quaternion algebra defined over the rational numbers.

#sd123
    
URL: sagemath#37164
Reported by: Sebastian Spindler
Reviewer(s): grhkm21, Sebastian Spindler
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

9 participants