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: sage-8.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 follow-up 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 slelievre

  • Branch set to u/slelievre/abs_universal_cyclotomic

comment:2 Changed 4 months ago by slelievre

  • Authors set to Samuel Lelièvre
  • Branch u/slelievre/abs_universal_cyclotomic deleted
  • Status changed from new to needs_review

comment:3 Changed 4 months ago by slelievre

  • Branch set to u/slelievre/abs_universal_cyclotomic
  • Commit set to 6ea0ad384a38f09655ef49039c881d892d62a0c2

Please review. Does this needs to go through deprecation?


New commits:

6ea0ad3Rename abs to __abs__ in UniversalCyclotomicField

comment:4 follow-up: Changed 4 months ago by slelievre

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 jdemeyer

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 chapoton

and f.abs() should still work, so please keep an alias and a doctest

comment:7 Changed 4 months ago by git

  • Commit changed from 6ea0ad384a38f09655ef49039c881d892d62a0c2 to 76674bde72747a638a2b4d20198ae04a55ec505f

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

76674bdAlias abs for __abs__ and extra doctests

comment:8 Changed 4 months ago by slelievre

Ready for review again with reviewer comments addressed.

comment:9 Changed 4 months ago by chapoton

  • 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 vbraun

  • Branch changed from u/slelievre/abs_universal_cyclotomic to 76674bde72747a638a2b4d20198ae04a55ec505f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.