Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Absolute value function for scalar fields #32396

Closed
mjungmath opened this issue Aug 18, 2021 · 17 comments
Closed

Absolute value function for scalar fields #32396

mjungmath opened this issue Aug 18, 2021 · 17 comments

Comments

@mjungmath
Copy link

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.

CC: @egourgoulhon @tscrim

Component: manifolds

Author: Michael Jung

Branch/Commit: 2095de3

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32396

@mjungmath mjungmath added this to the sage-9.5 milestone Aug 18, 2021
@mjungmath
Copy link
Author

@mjungmath
Copy link
Author

Commit: fcd64c9

@mjungmath
Copy link
Author

New commits:

fcd64c9Trac #32396: add `__abs__` to chart_func and scalarfield

@mjungmath

This comment has been minimized.

@mjungmath

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Aug 19, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 19, 2021

comment:5

LGTM.

@mjungmath
Copy link
Author

comment:6

Thank you for the review!

@egourgoulhon
Copy link
Member

comment:7

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

@mjungmath
Copy link
Author

comment:8

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

@egourgoulhon
Copy link
Member

comment:9

Replying to @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?

@tscrim
Copy link
Collaborator

tscrim commented Aug 19, 2021

comment:10

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 19, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 19, 2021

Changed commit from fcd64c9 to 2095de3

@mjungmath
Copy link
Author

comment:12

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

@tscrim
Copy link
Collaborator

tscrim commented Aug 19, 2021

comment:13

Thanks. I am setting back to a positive review.

@mkoeppe mkoeppe changed the title Abolute value function for scalar fields Absolute value function for scalar fields Aug 20, 2021
@vbraun
Copy link
Member

vbraun commented Sep 13, 2021

Changed branch from u/gh-mjungmath/abs_function_for_scalar_fields to 2095de3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants