#32396 closed enhancement (fixed)

Absolute value function for scalar fields

Reported by: Michael Jung Owned by:
Priority: major Milestone: sage-9.5
Component: manifolds Keywords:
Cc: Eric Gourgoulhon, Travis Scrimshaw Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 2095de3 (Commits, GitHub, GitLab) Commit: 2095de3117eb3a6cee9f4a928d0b8fd2e4496e85
Dependencies: Stopgaps:

Status badges

Description (last modified by Michael Jung)

Currently, the abs operator cannot be applied to scalar fields:

sage: M = Manifold(2, 'M', structure='topological')
sage: X.<x,y> = M.chart()
sage: f = M.scalar_field({X: x*y}, name='f', latex_name=r"\Phi")
sage: abs(f)
Traceback (most recent call last)
...
TypeError: bad operand type for abs(): 'ScalarFieldAlgebra_with_category.element_class'

In this ticket, we add this feature.

Change History (15)

comment:1 Changed 16 months ago by Michael Jung

Branch: u/gh-mjungmath/abs_function_for_scalar_fields

comment:2 Changed 16 months ago by Michael Jung

Commit: fcd64c9c1ecc5e68ec5e42125f7d403138fe7b0a
Status: newneeds_review

New commits:

fcd64c9Trac #32396: add __abs__ to chart_func and scalarfield

comment:3 Changed 16 months ago by Michael Jung

Description: modified (diff)

comment:4 Changed 16 months ago by Michael Jung

Description: modified (diff)

comment:5 Changed 16 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

LGTM.

comment:6 Changed 16 months ago by Michael Jung

Thank you for the review!

comment:7 Changed 16 months ago by Eric Gourgoulhon

Thanks for implementing this. Any reason for the choice of Abs rather than abs for the function name?

Last edited 16 months ago by Eric Gourgoulhon (previous) (diff)

comment:8 Changed 16 months ago by Michael Jung

No particular reason. Do you prefer abs or maybe |.|?

Last edited 16 months ago by Michael Jung (previous) (diff)

comment:9 in reply to:  8 Changed 16 months ago by Eric Gourgoulhon

Replying to gh-mjungmath:

No particular reason. Do you prefer abs or maybe |.|?

I don't have any strong preference. abs sounds more inline with other function names in Sage, while |.| is closer to mathematical notation, so I would slightly prefer the latter. Travis, what do you think?

comment:10 Changed 16 months ago by Travis Scrimshaw

I am slightly worried about possible ambiguity with |.|. I might go with abs over Abs just because of \lvert compared to \lVert and to match Sages's abs. However, I didn't (and still don't) really care for which one as it is clear documentation-wise what it is doing (and hidden from the causal user). So I am not the person to ask for a real opinion on this.

comment:11 Changed 16 months ago by git

Commit: fcd64c9c1ecc5e68ec5e42125f7d403138fe7b0a2095de3117eb3a6cee9f4a928d0b8fd2e4496e85
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2095de3Trac #32396: adjust abs function name to general Sage convention

comment:12 Changed 16 months ago by Michael Jung

Alright, I changed it to abs with small letters. I find it a strong argument to say that this follows general Sage convention.

comment:13 Changed 16 months ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Thanks. I am setting back to a positive review.

comment:14 Changed 16 months ago by Matthias Köppe

Summary: Abolute value function for scalar fieldsAbsolute value function for scalar fields

comment:15 Changed 15 months ago by Volker Braun

Branch: u/gh-mjungmath/abs_function_for_scalar_fields2095de3117eb3a6cee9f4a928d0b8fd2e4496e85
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.