Opened 6 years ago
Closed 6 years ago
#21105 closed enhancement (fixed)
abs for number field element
Reported by:  Vincent Delecroix  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  number fields  Keywords:  
Cc:  Matthias Köppe  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  15d160e (Commits, GitHub, GitLab)  Commit:  15d160e2ec536e988494080f187e164f26ddd291 
Dependencies:  Stopgaps: 
Description (last modified by )
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 6 years ago by
Description:  modified (diff) 

comment:2 Changed 6 years ago by
Branch:  → u/mkoeppe/abs_for_number_field_element 

comment:3 Changed 6 years ago by
Commit:  → 33e5f4cc2b7b4312cac49d5d9c8c07b876a212de 

comment:4 Changed 6 years ago by
Authors:  → Matthias Koeppe 

Status:  new → needs_review 
comment:5 Changed 6 years ago by
It is weird to have abs(a)
and a.abs()
behaving differently, don't you think?
comment:7 Changed 6 years ago by
By the way, should perhaps the print representation of a number field indicate whether it's embedded or not?
comment:8 followup: 9 Changed 6 years ago by
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 followup: 10 Changed 6 years ago by
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 followup: 12 Changed 6 years ago by
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 6 years ago by
Commit:  33e5f4cc2b7b4312cac49d5d9c8c07b876a212de → 0c31b4956e5660faafa242bc089b7044e39db65e 

Branch pushed to git repo; I updated commit sha1. New commits:
0c31b49  Trac 21105: Make x.abs() behave the same as abs(x) for real embedded number field elements

comment:12 Changed 6 years ago by
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:14 Changed 6 years ago by
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 6 years ago by
Commit:  0c31b4956e5660faafa242bc089b7044e39db65e → ba1fbec6a975d37711181292157c4866ca3c148e 

Branch pushed to git repo; I updated commit sha1. New commits:
ba1fbec  Fix indentation of examples

comment:16 Changed 6 years ago by
Reviewers:  → Vincent Delecroix 

comment:17 followup: 19 Changed 6 years ago by
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
andprec
).
comment:18 Changed 6 years ago by
Commit:  ba1fbec6a975d37711181292157c4866ca3c148e → f7b105fac07ddffadf352fa3031746d187ba7d28 

comment:19 Changed 6 years ago by
Replying to vdelecroix:
you should also test the output with arguments (both
i
andprec
).
Done.
I've also changed what happens when prec
is provided. I think it makes more sense now.
comment:20 Changed 6 years ago by
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 6 years ago by
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 followup: 24 Changed 6 years ago by
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 6 years ago by
Commit:  f7b105fac07ddffadf352fa3031746d187ba7d28 → c7634ff5562f2cc59288f0986c8538a16d37f39e 

Branch pushed to git repo; I updated commit sha1. New commits:
c7634ff  NumberFieldElement.abs: Clarify return type

comment:24 Changed 6 years ago by
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 followup: 27 Changed 6 years ago by
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 6 years ago by
Commit:  c7634ff5562f2cc59288f0986c8538a16d37f39e → 15d160e2ec536e988494080f187e164f26ddd291 

Branch pushed to git repo; I updated commit sha1. New commits:
15d160e  NumberFieldElement.abs: Clarify that CDF is not used

comment:27 Changed 6 years ago by
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 6 years ago by
Milestone:  sage7.3 → sage7.4 

Status:  needs_review → positive_review 
Thanks for the fix!
comment:29 Changed 6 years ago by
Branch:  u/mkoeppe/abs_for_number_field_element → 15d160e2ec536e988494080f187e164f26ddd291 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Add test for #21105