Opened 16 months ago
Closed 15 months ago
#32396 closed enhancement (fixed)
Absolute value function for scalar fields
Reported by:  Michael Jung  Owned by:  

Priority:  major  Milestone:  sage9.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: 
Description (last modified by )
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
Branch:  → u/ghmjungmath/abs_function_for_scalar_fields 

comment:2 Changed 16 months ago by
Commit:  → fcd64c9c1ecc5e68ec5e42125f7d403138fe7b0a 

Status:  new → needs_review 
comment:3 Changed 16 months ago by
Description:  modified (diff) 

comment:4 Changed 16 months ago by
Description:  modified (diff) 

comment:5 Changed 16 months ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
LGTM.
comment:7 Changed 16 months ago by
Thanks for implementing this. Any reason for the choice of Abs
rather than abs
for the function name?
comment:8 followup: 9 Changed 16 months ago by
No particular reason. Do you prefer abs
or maybe .
?
comment:9 Changed 16 months ago by
Replying to ghmjungmath:
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
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 documentationwise 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
Commit:  fcd64c9c1ecc5e68ec5e42125f7d403138fe7b0a → 2095de3117eb3a6cee9f4a928d0b8fd2e4496e85 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
2095de3  Trac #32396: adjust abs function name to general Sage convention

comment:12 Changed 16 months ago by
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
Status:  needs_review → positive_review 

Thanks. I am setting back to a positive review.
comment:14 Changed 16 months ago by
Summary:  Abolute value function for scalar fields → Absolute value function for scalar fields 

comment:15 Changed 15 months ago by
Branch:  u/ghmjungmath/abs_function_for_scalar_fields → 2095de3117eb3a6cee9f4a928d0b8fd2e4496e85 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Trac #32396: add __abs__ to chart_func and scalarfield