Opened 9 months ago
Closed 8 months ago
#32396 closed enhancement (fixed)
Absolute value function for scalar fields
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.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/gh-mjungmath/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 follow-up: ↓ 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 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 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 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 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/gh-mjungmath/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