Opened 3 years ago

Closed 3 years ago

#21105 closed enhancement (fixed)

abs for number field element

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.4
Component: number fields Keywords:
Cc: mkoeppe Merged in:
Authors: Matthias Koeppe Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 15d160e (Commits) Commit: 15d160e2ec536e988494080f187e164f26ddd291
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

For a number field element a, abs(a) currently returns a floating point real number

sage: K.<cbrt2> = NumberField(x^3 - 2, 'a', embedding=1.26)
sage: abs(cbrt2)
1.25992104989487
sage: parent(_)
Real Field with 53 bits of precision

If a coercion embedding is defined with value in RR, the absolute value can be defined internally as it is the case for AA

sage: abs(AA(2).sqrt() - AA(2))
0.5857864376269049?

and also for quadratic extensions

sage: K.<sqrt2> = NumberField(x^2 - 2, 'a', embedding=1.41)
sage: abs(sqrt2)
sqrt2

We propose to change the behavior for embedded number fields. Namely make abs an internal operation.

As a (minor) consequence of the current behavior, the inequalities from sage.geometry.polyhedron.representation.Inequality gets badly displayed

sage: K.<cbrt2> = NumberField(x^3 - 2, 'a', embedding=1.26)
sage: P = Polyhedron(vertices=[(1,1,cbrt2),(cbrt2,1,1)])
sage: P.inequalities()
(An inequality (-cbrt2^2 - cbrt2 - 1, 0, 0) x + 4.84732210186307 >= 0,
 An inequality (cbrt2^2 + cbrt2 + 1, 0, 0) x - 3.84732210186307 >= 0)

Change History (29)

comment:1 Changed 3 years ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 3 years ago by mkoeppe

  • Branch set to u/mkoeppe/abs_for_number_field_element

comment:3 Changed 3 years ago by git

  • Commit set to 33e5f4cc2b7b4312cac49d5d9c8c07b876a212de

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

33e5f4cAdd test for #21105

comment:4 Changed 3 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Status changed from new to needs_review

comment:5 Changed 3 years ago by vdelecroix

It is weird to have abs(a) and a.abs() behaving differently, don't you think?

comment:6 follow-up: Changed 3 years ago by mkoeppe

OK, I'll change a.abs() too.

comment:7 Changed 3 years ago by mkoeppe

By the way, should perhaps the print representation of a number field indicate whether it's embedded or not?

comment:8 follow-up: Changed 3 years ago by vdelecroix

That's indeed an issue. But it is hard to solve since you are interested at the same time in both the algebraic expression and the embedded value.

More generally, instead of having if/else in many methods I think that a better solution would be to have a dedicated class for real embedded element (whose semantic would be to follow standard method of real numbers, possibly having methods like .cos(), .exp() and such). But that is another question that will not be solved by the ticket.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by mkoeppe

Replying to vdelecroix:

That's indeed an issue. But it is hard to solve since you are interested at the same time in both the algebraic expression and the embedded value.

True, but I meant the print representation of the field itself (the parent), not an element.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by vdelecroix

Replying to mkoeppe:

Replying to vdelecroix:

That's indeed an issue. But it is hard to solve since you are interested at the same time in both the algebraic expression and the embedded value.

True, but I meant the print representation of the field itself (the parent), not an element.

+1. Put me in cc if you open a ticket.

comment:11 Changed 3 years ago by git

  • Commit changed from 33e5f4cc2b7b4312cac49d5d9c8c07b876a212de to 0c31b4956e5660faafa242bc089b7044e39db65e

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

0c31b49Trac 21105: Make x.abs() behave the same as abs(x) for real embedded number field elements

comment:12 in reply to: ↑ 10 Changed 3 years ago by mkoeppe

Replying to vdelecroix:

Replying to mkoeppe:

Replying to vdelecroix:

That's indeed an issue. But it is hard to solve since you are interested at the same time in both the algebraic expression and the embedded value.

True, but I meant the print representation of the field itself (the parent), not an element.

+1. Put me in cc if you open a ticket.

See #21161.

comment:13 in reply to: ↑ 6 Changed 3 years ago by mkoeppe

Replying to mkoeppe:

OK, I'll change a.abs() too.

Done.

comment:14 Changed 3 years ago by vdelecroix

This is missing indentation

Check that for fields with real coercion embeddings, absolute
values are in the same field (:trac:`21105`)::

sage: x = polygen(ZZ)      <---- test should be indented

comment:15 Changed 3 years ago by git

  • Commit changed from 0c31b4956e5660faafa242bc089b7044e39db65e to ba1fbec6a975d37711181292157c4866ca3c148e

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

ba1fbecFix indentation of examples

comment:16 Changed 3 years ago by mkoeppe

  • Reviewers set to Vincent Delecroix

comment:17 follow-up: Changed 3 years ago by vdelecroix

After

            sage: x = polygen(ZZ)
            sage: f = x^3 - x - 1
            sage: K.<b> = NumberField(f, embedding=1.3)
            sage: b.abs()
            b

you should also test the output with arguments (both i and prec).

Last edited 3 years ago by vdelecroix (previous) (diff)

comment:18 Changed 3 years ago by git

  • Commit changed from ba1fbec6a975d37711181292157c4866ca3c148e to f7b105fac07ddffadf352fa3031746d187ba7d28

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

586ce66NumberFieldElement.abs: Add doctest
f7b105freal embedded numberfield abs: Return complex if prec is given

comment:19 in reply to: ↑ 17 Changed 3 years ago by mkoeppe

Replying to vdelecroix:

you should also test the output with arguments (both i and prec).

Done. I've also changed what happens when prec is provided. I think it makes more sense now.

comment:20 Changed 3 years ago by vdelecroix

If prec=None and there is no real embedded, I would actually return an element of AA (the real algebraic field). But it becomes beyond the scope of the ticket... (and breaks even more the previous behavior).

comment:21 Changed 3 years ago by mkoeppe

Yes, I guess that would make sense; but I agree it's too much for this ticket.

By the way, why does it use complex fields instead of real fields for the result?

comment:22 follow-up: Changed 3 years ago by vdelecroix

It does not: the method uses complex field for the embedding. But the result of .abs() on a complex number is real

sage: CC(0,1).abs().parent()
Real Field with 53 bits of precision

comment:23 Changed 3 years ago by git

  • Commit changed from f7b105fac07ddffadf352fa3031746d187ba7d28 to c7634ff5562f2cc59288f0986c8538a16d37f39e

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

c7634ffNumberFieldElement.abs: Clarify return type

comment:24 in reply to: ↑ 22 Changed 3 years ago by mkoeppe

Replying to vdelecroix:

It does not: the method uses complex field for the embedding. But the result of .abs() on a complex number is real

Ah, thanks. I've reworded the docstring to clarify.

comment:25 follow-up: Changed 3 years ago by vdelecroix

I don't understand this sentence from the doc of abs

If ``prec`` is ``None`` or 53, then the complex double field is
used; otherwise the arbitrary precision (but slow) complex
field is used.  The result is in the corresponding real field.

The "complex double field" refers to I guess "CDF" but which is actually not used! I would rather remove that sentence and explain that the default precision is 53.

comment:26 Changed 3 years ago by git

  • Commit changed from c7634ff5562f2cc59288f0986c8538a16d37f39e to 15d160e2ec536e988494080f187e164f26ddd291

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

15d160eNumberFieldElement.abs: Clarify that CDF is not used

comment:27 in reply to: ↑ 25 Changed 3 years ago by mkoeppe

Replying to vdelecroix:

I don't understand this sentence from the doc of abs

If ``prec`` is ``None`` or 53, then the complex double field is
used; otherwise the arbitrary precision (but slow) complex
field is used.  The result is in the corresponding real field.

The "complex double field" refers to I guess "CDF" but which is actually not used! I would rather remove that sentence and explain that the default precision is 53.

Thanks for catching this.

comment:28 Changed 3 years ago by vdelecroix

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to positive_review

Thanks for the fix!

comment:29 Changed 3 years ago by vbraun

  • Branch changed from u/mkoeppe/abs_for_number_field_element to 15d160e2ec536e988494080f187e164f26ddd291
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.