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

abs for number field element #21105

Closed
videlec opened this issue Jul 27, 2016 · 35 comments
Closed

abs for number field element #21105

videlec opened this issue Jul 27, 2016 · 35 comments

Comments

@videlec
Copy link
Contributor

videlec commented Jul 27, 2016

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)

CC: @mkoeppe

Component: number fields

Author: Matthias Koeppe

Branch/Commit: 15d160e

Reviewer: Vincent Delecroix

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

@videlec videlec added this to the sage-7.3 milestone Jul 27, 2016
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

Commit: 33e5f4c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

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

33e5f4cAdd test for #21105

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

Author: Matthias Koeppe

@videlec
Copy link
Contributor Author

videlec commented Aug 3, 2016

comment:5

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

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

comment:6

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

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

comment:7

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

@videlec
Copy link
Contributor Author

videlec commented Aug 3, 2016

comment:8

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.

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

comment:9

Replying to @videlec:

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.

@videlec
Copy link
Contributor Author

videlec commented Aug 3, 2016

comment:10

Replying to @mkoeppe:

Replying to @videlec:

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

Changed commit from 33e5f4c to 0c31b49

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

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

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

comment:12

Replying to @videlec:

Replying to @mkoeppe:

Replying to @videlec:

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.

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2016

comment:13

Replying to @mkoeppe:

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

Done.

@videlec
Copy link
Contributor Author

videlec commented Aug 3, 2016

comment:14

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

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

ba1fbecFix indentation of examples

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

Changed commit from 0c31b49 to ba1fbec

@mkoeppe
Copy link
Member

mkoeppe commented Aug 5, 2016

Reviewer: Vincent Delecroix

@videlec
Copy link
Contributor Author

videlec commented Aug 5, 2016

comment:17

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

Changed commit from ba1fbec to f7b105f

@mkoeppe
Copy link
Member

mkoeppe commented Aug 5, 2016

comment:19

Replying to @videlec:

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.

@videlec
Copy link
Contributor Author

videlec commented Aug 5, 2016

comment:20

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

@mkoeppe
Copy link
Member

mkoeppe commented Aug 5, 2016

comment:21

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?

@videlec
Copy link
Contributor Author

videlec commented Aug 5, 2016

comment:22

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

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

c7634ffNumberFieldElement.abs: Clarify return type

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

Changed commit from f7b105f to c7634ff

@mkoeppe
Copy link
Member

mkoeppe commented Aug 5, 2016

comment:24

Replying to @videlec:

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.

@videlec
Copy link
Contributor Author

videlec commented Aug 5, 2016

comment:25

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

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

15d160eNumberFieldElement.abs: Clarify that CDF is not used

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2016

Changed commit from c7634ff to 15d160e

@mkoeppe
Copy link
Member

mkoeppe commented Aug 5, 2016

comment:27

Replying to @videlec:

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.

@videlec
Copy link
Contributor Author

videlec commented Aug 5, 2016

comment:28

Thanks for the fix!

@vbraun
Copy link
Member

vbraun commented Aug 7, 2016

Changed branch from u/mkoeppe/abs_for_number_field_element to 15d160e

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

3 participants