Opened 3 years ago
Closed 3 years ago
#21657 closed defect (invalid)
Import abs in functions/all.py
Reported by: | paulmasson | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-duplicate/invalid/wontfix |
Component: | symbolics | Keywords: | |
Cc: | Merged in: | ||
Authors: | Paul Masson | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/paulmasson/import_abs_in_functions_all_py | Commit: | b36e1c415161ffaa93db4975fd8b69b4374a7a5a |
Dependencies: | Stopgaps: |
Description (last modified by )
Since abs
is an explicit alias of abs_symbolic
it should be explicitly imported from functions/other.py
Change History (11)
comment:1 Changed 3 years ago by
- Branch set to u/paulmasson/import_abs_in_functions_all_py
comment:2 Changed 3 years ago by
- Commit set to b36e1c415161ffaa93db4975fd8b69b4374a7a5a
- Component changed from PLEASE CHANGE to symbolics
- Description modified (diff)
- Priority changed from major to minor
- Status changed from new to needs_review
- Type changed from PLEASE CHANGE to defect
comment:3 follow-up: ↓ 4 Changed 3 years ago by
Patchbot:
sage -t --long src/sage/matrix/matrix2.pyx # 5 doctests failed sage -t --long src/sage/symbolic/expression.pyx # 1 doctest failed sage -t --long src/sage/rings/continued_fraction.py # 2 doctests failed sage -t --long src/sage/rings/number_field/number_field_element_quadratic.pyx # 1 doctest failed sage -t --long src/sage/modules/free_module_element.pyx # 2 doctests failed sage -t --long src/sage/stats/distributions/discrete_gaussian_lattice.py # 2 doctests failed sage -t --long src/sage/matrix/matrix_symbolic_dense.pyx # 2 doctests failed sage -t --long src/sage/matrix/matrix_double_dense.pyx # 2 doctests failed sage -t --long src/sage/misc/sage_input.py # 1 doctest failed sage -t --long src/sage/libs/mpmath/ext_main.pyx # 1 doctest failed
comment:4 in reply to: ↑ 3 Changed 3 years ago by
Commenting on some of the doctest failures...
sage -t --long --warn-long 35.3 src/sage/symbolic/expression.pyx ********************************************************************** File "src/sage/symbolic/expression.pyx", line 6868, in sage.symbolic.expression.Expression.__abs__ Failed example: abs(SR(-5),hold=True) Expected: Traceback (most recent call last): ... TypeError: abs() takes no keyword arguments Got: abs(-5)
This looks like the correct answer so just adapt the doctest.
File "src/sage/rings/continued_fraction.py", line 500, in sage.rings.continued_fraction.ContinuedFraction_base.__abs__ Failed example: abs(a) Exception raised: ... TypeError: cannot coerce arguments: no canonical coercion from <class 'sage.rings.continued_fraction.ContinuedFraction_periodic'> to Symbolic Ring
Most fails are of this type. I think this is related to #17790.
sage -t --long src/sage/libs/mpmath/ext_main.pyx ... File "sage/symbolic/function.pyx", line 766, in sage.symbolic.function.Function._eval_mpmath_ (build/cythonized/sage/symbolic/function.cpp:8582) res = self(*args) File "sage/symbolic/function.pyx", line 979, in sage.symbolic.function.BuiltinFunction.__call__ (build/cythonized/sage/symbolic/function.cpp:11061) return custom(*args) File "sage/symbolic/function.pyx", line 766, in sage.symbolic.function.Function._eval_mpmath_ (build/cythonized/sage/symbolic/function.cpp:8582) res = self(*args) File "sage/symbolic/function.pyx", line 979, in sage.symbolic.function.BuiltinFunction.__call__ (build/cythonized/sage/symbolic/function.cpp:11061) return custom(*args) ... RuntimeError: maximum recursion depth exceeded while calling a Python object
This one is different, it looks like a bug in BuiltinFunction._eval_mpmath_()
.
comment:5 follow-up: ↓ 8 Changed 3 years ago by
Can we not merge this ticket and close it as invalid?
I think the alias should be interpreted the other way around: sage.functions.other.abs
is bound because there's no name clash and this is the most logical name. In addition, there's sage.functions.other.abs_symbolic
which avoids the nameclash with python's builtin abs
and is therefore suitable for importing into the global namespace (does it need to be there, though?).
comment:6 follow-up: ↓ 7 Changed 3 years ago by
My sage-devel post repeated here:
Expression.__abs__()
directly calls Pynac's abs_eval
while abs_symbolic
goes through GinacFunction
's Python code first, e.g.:
sage: abs_symbolic(x,x) ... TypeError: Symbolic function abs takes exactly 1 arguments (2 given) sage: abs(x,x) ... TypeError: abs() takes exactly one argument (2 given)
so there is a difference that might show with other usages too.
comment:7 in reply to: ↑ 6 Changed 3 years ago by
Replying to rws:
My sage-devel post repeated here:
Expression.__abs__()
directly calls Pynac'sabs_eval
whileabs_symbolic
goes throughGinacFunction
's Python code first
You can change that if you want to, but I'm not sure if one or the other has an advantage.
e.g.:
sage: abs_symbolic(x,x) ... TypeError: Symbolic function abs takes exactly 1 arguments (2 given) sage: abs(x,x) ... TypeError: abs() takes exactly one argument (2 given)
That doesn't confirm the statement you made above. Python's abs
does a check on the number of parameters before dispatching. It has to because it dispatches through a slot in PyNumberMethods
, going to a c-function that accepts only one argument (meaning that for instance on cython types, abs(c)
can work just as fast as len(c)
etc: these methods are resolved via a pointer on the type; no method resolution required)
So the error traceback of abs(x,x)
and abs_symbolic(x,x)
will be different even if you chance Expression.__abs__
.
so there is a difference that might show with other usages too.
These difference have a good reason, though, and are evidence of an efficiency in builtin.abs
that we do not want to lose (e.g., people doing numerical work are going to be quite upset if they don't get the usual abs
in sage anymore and have to pay the price of full method resolution and a GinacFunction
).
Differences between toplevel functions and symbolic functions happen more often, even for things that are in control of sage:
sage: F=integrate(function('f')(x),x,0,1); F integrate(f(x), x, 0, 1) sage: integrate is F.operator() False sage: type(integrate) <type 'function'> sage: type(F.operator()) <class 'sage.symbolic.integration.integral.DefiniteIntegral'> sage: integrate is sage.symbolic.integration.integral.integrate False
comment:8 in reply to: ↑ 5 ; follow-up: ↓ 10 Changed 3 years ago by
Replying to nbruin:
Can we not merge this ticket and close it as invalid?
I really didn't expect this simple change to cause so much trouble, so yes let's close it. Thanks for the feedback.
comment:9 Changed 3 years ago by
- Milestone changed from sage-7.4 to sage-duplicate/invalid/wontfix
comment:10 in reply to: ↑ 8 Changed 3 years ago by
- Status changed from needs_review to positive_review
Replying to paulmasson:
I really didn't expect this simple change to cause so much trouble, so yes let's close it. Thanks for the feedback.
:-) Hopefully you enjoyed learning something about python and sage in the process.
comment:11 Changed 3 years ago by
- Resolution set to invalid
- Status changed from positive_review to closed
New commits:
Import abs