Comparisons in quadratic number field #13213

videlec · 2012-07-08T18:45:16Z

The order of quadratic field with specified embedding is not induced from the order of RR and CC. More precisely we have

sage: K.<sqrt2> = NumberField(x^2 - 2, 'sqrt2', embedding=1.4142)
sage: sqrt2 < 100
False

sage: K.<s> = NumberField(x^3 - 2, 's', embedding=1.26)
sage: s < 100
False

which is not compatible with the order of RR and

sage: K.<i> = QuadraticField(-1)
sage: i > 1
True
sage: 1 > i
True

which is not compatible (!) with the order of CC.

The ticket implements the order for quadratic field (for which comparisons are made using only operations on integers).

Note:

this patch is partly a duplicate because of comparison with quadratic number field elements #7160
the modifications for quadratic field field modify the behavior of many commands (especially about output order).
the patch introduces a new attribute to NumberFieldElement_quadratic called standard_embedding (of type bint)

There is no significant lost of speed as shown on the following timings.

sage: K.<sqrt2> = QuadraticField(2, 'sqrt2', embedding=1.4142)
sage: a = (3*sqrt2 + 18)/7
sage: b = (5*sqrt2 + 14)/5
sage: %timeit a < b
1000000 loops, best of 3: 408 ns per loop

sage: K.<s> = QuadraticField(-2)
sage: a = 3*s + 2/4
sage: b = 5/7*s + 1/3
sage: %timeit a < b
1000000 loops, best of 3: 352 ns per loop

Timings without the patch

sage: sage: K.<sqrt2> = QuadraticField(2,'sqrt2',embedding=1.4142)
sage: a = (3*sqrt2 + 18)/7
sage: b = (5*sqrt2 + 14)/5
sage: %timeit a < b
1000000 loops, best of 3: 393 ns per loop

sage: K.<s> = QuadraticField(-2)
sage: a = 3*s+2/4
sage: b = 5/7*s+1/3
sage: %timeit a < b
1000000 loops, best of 3: 344 ns per loop

Apply:

attachment: trac_13213-quadratic_field_comparison.patch

Component: number fields

Keywords: real number field, comparison

Author: Vincent Delecroix

Reviewer: Burcin Erocal, Volker Braun

Merged: sage-5.10.beta4

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

The text was updated successfully, but these errors were encountered:

a-andre · 2012-07-08T19:42:26Z

comment:1

I had a short look at your patch.
Could you use Python 3 compatible syntax to raise errors. E.g.

raise ValueError("absolute value not implemented for complex embeddings")

a-andre · 2012-07-08T19:42:26Z

Changed author from vdelecroix to Vincent Delecroix

burcin · 2012-07-10T17:04:55Z

comment:5

Thanks again for the patch. A few comments:

reimplementing the comparison of quadratic number field elements is a well defined unit of change itself. I suggest moving floor() and ceil() implementations to a separate patch, even a different ticket.
Do you really need to use rich comparisons? Working only with __cmp__ should simplify the case comparisons before the return statements and avoid the new python_object.pxi include.
I guess speed could be further improved by using an approximation (with doubles say) first and falling back to the exact computation if the results are too close.
left.D is of type Integer. Cython tries to do the right thing for <unsigned int> left.D, but it would be better to use mpz_mul() with left.D.value.
mpz_sgn() returns 0 if the argument is 0. You don't seem to cover this case, though I couldn't find any example that returns wrong results.
To avoid code duplication, it might be better change the if statement to:

        if mpz_sgn(i)*mpz_sgn(j) == 1:
            # both i and j are positive or negative
            mpz_mul(i, i, i)
            mpz_mul(j, j, j)
            mpz_mul_ui(j, j, <unsigned int> left.D)

            test = mpz_cmp(i, j)
            if mpz_sgn(i) == -1:
                # both i and j are negative
                test *= -1

burcin · 2012-07-10T17:04:55Z

Reviewer: Burcin Erocal

videlec · 2012-07-11T07:37:03Z

comment:6

reimplementing the comparison of quadratic number field elements is a well defined unit of change itself. I suggest moving floor() and ceil() implementations to a separate patch, even a different ticket.

left.D is of type Integer. Cython tries to do the right thing for <unsigned int> left.D, but it would be better to use mpz_mul() with left.D.value.

Done and done.

Do you really need to use rich comparisons? Working only with __cmp__ should simplify the case comparisons before the return statements and avoid the new python_object.pxi include.

We loose speed (~1 micro sec) by doing this. I quickly look at the code of comparison for Integer. The comparison is directly implemented inside __richcmp__ and avoid as much as possible the coercion machinery (ie the type of the input is check with the cython function PY_TYPE_CHECK). Do you think we should proceed that way ?

I guess speed could be further improved by using an approximation (with doubles say) first and falling back to the exact computation if the results are too close.

What do you mean by too close ? How do I certify that my float comparison is accurate enough ?
Moreover, if we proceed that way we have to perform conversion from mpz_t to float (or double or real_mpfr). A way it may work is to look first if a,b, D and denom have reasonable values (recall that mpz_t have unlimited precision) and if yes, convert it to double and try to compare the arguments.
Last, I'm not sure the gain of speed would be terrific. Comparison requires only 7 integer operations.

mpz_sgn() returns 0 if the argument is 0. You don't seem to cover this case, though I couldn't find any example that returns wrong results.

Actually there is no problem since we compute the sign of an expression of the form a^2 + b^2 D where a and b are both integers (and one of them is non null) and D is a square free integer. Hence the sign is never 0.

To avoid code duplication, it might be better change the if statement to:

        if mpz_sgn(i)*mpz_sgn(j) == 1:
            # both i and j are positive or negative
            mpz_mul(i, i, i)
            mpz_mul(j, j, j)
            mpz_mul_ui(j, j, <unsigned int> left.D)

            test = mpz_cmp(i, j)
            if mpz_sgn(i) == -1:
                # both i and j are negative
                test *= -1

Sure but that way we call twice mpz_sgn(i)... I may change it if you prefer.

Actually, the part where a lot of time is lost is the check for embedding

if not RDF.has_coerce_map_from(left._parent):
    ...

By replacing it by

if left.D < 0:
   ...

I pass from 4 micro second to 2 micro seconds. That way comparison without embedding for positive discriminant will be equivalent to comparison with the standard embedding. This is in the last version of the patch. What do you think about that ?

videlec · 2012-07-18T11:59:40Z

comment:8

The code for comparison introduces a small bug as it ask to the parent whether the embedding is standard or not (via the boolean attribute _standard_embedding). But the latter was not defined for Cyclotomic field of order 3,4,6 (which are the ones which are quadratic). In that case, the attribute _standard_embedding is created at the initialization of the cyclotomic field.

The new patch takes care of this.

videlec · 2012-08-04T12:38:37Z

comment:10

Because there are many parents for quadratic elements, I was obliged to test whether the parent has a method '_standard_embedding' or not. I correct all tests (many output have changed) and waiting for patchbot and reviewer!

videlec · 2012-08-08T09:14:28Z

comment:12

I corrected the two last tests that have failed...

jdemeyer · 2012-08-08T12:42:37Z

comment:13

How does this patch here relate to #7160?

jdemeyer · 2012-08-08T12:45:14Z

comment:14

In the ticket title "real number field", should it say "quadratic number field" instead?

videlec · 2012-08-08T13:30:35Z

comment:15

Hi Jeroen,

From the last comment of Burcin on #7160, it seems to me that we will close #7160 as a duplicate. On the other hand, an other patch is coming for comparisons of general number field. Would you prefer to have it on a separate ticket ?

jdemeyer · 2012-08-08T13:43:04Z

comment:16

Replying to @videlec:

From the last comment of Burcin on #7160, it seems to me that we will close #7160 as a duplicate. On the other hand, an other patch is coming for comparisons of general number field. Would you prefer to have it on a separate ticket ?

I don't care very much, but I think it should be clear how the various tickets relate to eachother.

Also #10064 is related, at least it has #7160 as dependency.

tkluck · 2012-08-09T15:19:12Z

comment:18

Just had a quick look at your patch. How about using @cached_method as a decorator for _gen_approx? Then you don't have to implement caching yourself. It makes the code a little bit easier to read.

videlec · 2012-08-13T15:56:31Z

comment:19

Hello,

Actually, a lot of time is lost for asking questions to the parent. For quadratic number field, it is for asking about the attribute _standard_embedding. I build a new patch with a bint attribute added to NumberFieldElement_quadratic and the speed of comparison is divided by 10 (from 4 milliseconds to 400 nanoseconds). It will be updated as soon as possible.
As modifying the structure of NumberFieldElement_quadratic is a major change, I will split the patch into two parts : quadratic number fields and general number fields.

Vincent

videlec · 2013-05-10T16:10:20Z

comment:27

Last minute modification: more tests, more docs.

apply trac_13213-quadratic_field_comparison.patch

kcrisman · 2013-05-10T16:21:08Z

comment:28

#7545 is possibly a related ticket.

videlec · 2013-05-10T18:20:45Z

comment:29

There is a strange problem

sage: K.<sqrt2> = QuadraticField(2)
sage: 1/2 + sqrt2 > 0
False

videlec · 2013-05-10T19:10:02Z

comment:30

Replying to @videlec:

There is a strange problem

sage: K.<sqrt2> = QuadraticField(2)
sage: 1/2 + sqrt2 > 0
False

I got the problem... it was because there is a natural morphism implemented from the rational field to quadratic number field (sage.rings.number_field.number_field_element_quadratic.Q_to_quadratic_field_element) which uses a non properly initialized instance of a quadratic element !

videlec · 2013-05-10T19:10:40Z

comment:31

apply trac_13213-quadratic_field_comparison.patch

vbraun · 2013-05-14T11:11:17Z

comment:32

I get a doctest failure with sage-5.10.beta2:

File "devel/sage/doc/de/tutorial/tour_advanced.rst", line 483, in doc.de.tutorial.tour_advanced
Failed example:
    M.T(2).charpoly('x').factor()
Expected:
    (x - 2*zeta6 - 1) * (x - zeta6 - 2) * (x + zeta6 + 1)^2
Got:
    (x - zeta6 - 2) * (x - 2*zeta6 - 1) * (x + zeta6 + 1)^2
**********************************************************************
1 item had failures:
   1 of 136 in doc.de.tutorial.tour_advanced
    [118 tests, 1 failure, 1.44 s]

videlec · 2013-05-15T14:31:02Z

Attachment: trac_13213-quadratic_field_comparison.patch.gz

videlec · 2013-05-15T14:32:33Z

comment:33

Replying to @vbraun:

I get a doctest failure with sage-5.10.beta2:

File "devel/sage/doc/de/tutorial/tour_advanced.rst", line 483, in doc.de.tutorial.tour_advanced
Failed example:
    M.T(2).charpoly('x').factor()
Expected:
    (x - 2*zeta6 - 1) * (x - zeta6 - 2) * (x + zeta6 + 1)^2
Got:
    (x - zeta6 - 2) * (x - 2*zeta6 - 1) * (x + zeta6 + 1)^2
**********************************************************************
1 item had failures:
   1 of 136 in doc.de.tutorial.tour_advanced
    [118 tests, 1 failure, 1.44 s]

Thanks for that. I got the same error on my computer and change the patch accordingly. Why was patchbot happy with the former patch ?

apply trac_13213-quadratic_field_comparison.patch

vbraun · 2013-05-15T14:47:53Z

comment:34

The patchbot apparently doesn't check German docs. Enttäuschend! ;-)

vbraun · 2013-05-15T16:45:37Z

comment:35

All tests pass.

vbraun · 2013-05-15T16:45:37Z

Changed reviewer from Burcin Erocal to Burcin Erocal, Volker Braun

kcrisman · 2013-05-15T18:09:10Z

Changed work issues from rebase 5.10.beta2 to none

jdemeyer · 2013-05-17T06:32:23Z

Merged: sage-5.10.beta4

videlec added this to the sage-5.10 milestone Jul 8, 2012

videlec added c: number fields labels Jul 8, 2012

videlec self-assigned this Jul 8, 2012

This comment has been minimized.

Sign in to view

videlec added the s: needs review label Jul 14, 2012

This comment has been minimized.

Sign in to view

videlec added s: needs work and removed s: needs review labels Aug 4, 2012

videlec added s: needs review and removed s: needs work labels Aug 4, 2012

This comment has been minimized.

Sign in to view

videlec added s: needs work and removed s: needs review labels May 10, 2013

videlec added s: needs review and removed s: needs work labels May 10, 2013

vbraun added s: positive review and removed s: needs review labels May 15, 2013

jdemeyer removed the s: positive review label May 17, 2013

jdemeyer closed this as completed May 17, 2013

mwhansen mentioned this issue Jun 6, 2013

comparison with quadratic number field elements #7160

Closed

This was referenced May 9, 2013

Pynac comparison functions do not provide a SWO #9880

Closed

-1 in expression "is_positive". #10064

Closed

This was referenced Jan 16, 2023

abs floor ceil for quadratic number field #13256

Closed

faster coercion from Integer to Quadratic Number fields #14563

Closed

Refactor continued fractions #14567

Closed

This was referenced Jun 6, 2013

cleanup of doc in heegner.py #14595

Closed

cleanup of doc in ell_point.py #14609

Closed

videlec mentioned this issue Dec 29, 2015

Comparison of number field elements dependent of real embedding #17830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comparisons in quadratic number field #13213

Comparisons in quadratic number field #13213

videlec commented Jul 8, 2012

a-andre commented Jul 8, 2012

a-andre commented Jul 8, 2012

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

burcin commented Jul 10, 2012

burcin commented Jul 10, 2012

videlec commented Jul 11, 2012

This comment has been minimized.

videlec commented Jul 18, 2012

This comment has been minimized.

videlec commented Aug 4, 2012

videlec commented Aug 8, 2012

jdemeyer commented Aug 8, 2012

jdemeyer commented Aug 8, 2012

videlec commented Aug 8, 2012

jdemeyer commented Aug 8, 2012

This comment has been minimized.

tkluck commented Aug 9, 2012

videlec commented Aug 13, 2012

videlec commented May 10, 2013

kcrisman commented May 10, 2013

videlec commented May 10, 2013

videlec commented May 10, 2013

videlec commented May 10, 2013

vbraun commented May 14, 2013

videlec commented May 15, 2013

videlec commented May 15, 2013

vbraun commented May 15, 2013

vbraun commented May 15, 2013

vbraun commented May 15, 2013

kcrisman commented May 15, 2013

jdemeyer commented May 17, 2013

Comparisons in quadratic number field #13213

Comparisons in quadratic number field #13213

Comments

videlec commented Jul 8, 2012

a-andre commented Jul 8, 2012

a-andre commented Jul 8, 2012

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

burcin commented Jul 10, 2012

burcin commented Jul 10, 2012

videlec commented Jul 11, 2012

This comment has been minimized.

videlec commented Jul 18, 2012

This comment has been minimized.

videlec commented Aug 4, 2012

videlec commented Aug 8, 2012

jdemeyer commented Aug 8, 2012

jdemeyer commented Aug 8, 2012

videlec commented Aug 8, 2012

jdemeyer commented Aug 8, 2012

This comment has been minimized.

tkluck commented Aug 9, 2012

videlec commented Aug 13, 2012

videlec commented May 10, 2013

kcrisman commented May 10, 2013

videlec commented May 10, 2013

videlec commented May 10, 2013

videlec commented May 10, 2013

vbraun commented May 14, 2013

videlec commented May 15, 2013

videlec commented May 15, 2013

vbraun commented May 15, 2013

vbraun commented May 15, 2013

vbraun commented May 15, 2013

kcrisman commented May 15, 2013

jdemeyer commented May 17, 2013