Opened 4 months ago
Closed 4 months ago
#27201 closed enhancement (fixed)
Rename abs to __abs__ for UniversalCyclotomicField elements
Reported by:  slelievre  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  number fields  Keywords:  UniversalCyclotomicField, universal cyclotomic field 
Cc:  chapoton, slelievre, tscrim, vdelecroix  Merged in:  
Authors:  Samuel Lelièvre  Reviewers:  Jeroen Demeyer, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  76674bd (Commits)  Commit:  76674bde72747a638a2b4d20198ae04a55ec505f 
Dependencies:  Stopgaps: 
Description
This ticket renames abs
to __abs__
for UniversalCyclotomicField elements.
With this, calling abs(a)
calls a.__abs__()
, which was not the case with a.abs()
.
This is a followup to #26872 where abs
should have been called __abs__
.
See this report on Ask Sage and the comment there that #26872 did not fix the problem:
Change History (10)
comment:1 Changed 4 months ago by
 Branch set to u/slelievre/abs_universal_cyclotomic
comment:2 Changed 4 months ago by
 Branch u/slelievre/abs_universal_cyclotomic deleted
 Status changed from new to needs_review
comment:3 Changed 4 months ago by
 Branch set to u/slelievre/abs_universal_cyclotomic
 Commit set to 6ea0ad384a38f09655ef49039c881d892d62a0c2
comment:4 followup: ↓ 5 Changed 4 months ago by
I checked that this solves the problem reported on Ask Sage:
sage: a = E(8) sage: abs(a) 1 sage: v = vector([a]) sage: v.norm() 1
comment:5 in reply to: ↑ 4 Changed 4 months ago by
Replying to slelievre:
sage: a = E(8) sage: abs(a) 1 sage: v = vector([a]) sage: v.norm() 1
I think that this would be an excellent doctest to add.
Also: I prefer to see abs(X)
instead of X.__abs__()
in the tests.
comment:6 Changed 4 months ago by
and f.abs() should still work, so please keep an alias and a doctest
comment:7 Changed 4 months ago by
 Commit changed from 6ea0ad384a38f09655ef49039c881d892d62a0c2 to 76674bde72747a638a2b4d20198ae04a55ec505f
Branch pushed to git repo; I updated commit sha1. New commits:
76674bd  Alias abs for __abs__ and extra doctests

comment:8 Changed 4 months ago by
Ready for review again with reviewer comments addressed.
comment:9 Changed 4 months ago by
 Reviewers set to Jeroen Demeyer, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok
comment:10 Changed 4 months ago by
 Branch changed from u/slelievre/abs_universal_cyclotomic to 76674bde72747a638a2b4d20198ae04a55ec505f
 Resolution set to fixed
 Status changed from positive_review to closed
Please review. Does this needs to go through deprecation?
New commits:
Rename abs to __abs__ in UniversalCyclotomicField