Opened 9 months ago
Closed 8 months ago
#32396 closed enhancement (fixed)
Absolute value function for scalar fields
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim  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 9 months ago by
 Branch set to u/ghmjungmath/abs_function_for_scalar_fields
comment:2 Changed 9 months ago by
 Commit set to fcd64c9c1ecc5e68ec5e42125f7d403138fe7b0a
 Status changed from new to needs_review
comment:3 Changed 9 months ago by
 Description modified (diff)
comment:4 Changed 9 months ago by
 Description modified (diff)
comment:5 Changed 9 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:6 Changed 9 months ago by
Thank you for the review!
comment:7 Changed 9 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 9 months ago by
No particular reason. Do you prefer abs
or maybe .
?
comment:9 in reply to: ↑ 8 Changed 9 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 9 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 9 months ago by
 Commit changed from fcd64c9c1ecc5e68ec5e42125f7d403138fe7b0a to 2095de3117eb3a6cee9f4a928d0b8fd2e4496e85
 Status changed from positive_review to 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 9 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 9 months ago by
 Status changed from needs_review to positive_review
Thanks. I am setting back to a positive review.
comment:14 Changed 9 months ago by
 Summary changed from Abolute value function for scalar fields to Absolute value function for scalar fields
comment:15 Changed 8 months ago by
 Branch changed from u/ghmjungmath/abs_function_for_scalar_fields to 2095de3117eb3a6cee9f4a928d0b8fd2e4496e85
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #32396: add __abs__ to chart_func and scalarfield