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 paulmasson)

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 paulmasson

  • Branch set to u/paulmasson/import_abs_in_functions_all_py

comment:2 Changed 3 years ago by paulmasson

  • Authors set to Paul Masson
  • 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

New commits:

b36e1c4Import abs

comment:3 follow-up: Changed 3 years ago by rws

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 rws

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: Changed 3 years ago by nbruin

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: Changed 3 years ago by rws

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 nbruin

Replying to rws:

My sage-devel post repeated here: Expression.__abs__() directly calls Pynac's abs_eval while abs_symbolic goes through GinacFunction'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: Changed 3 years ago by paulmasson

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 paulmasson

  • Milestone changed from sage-7.4 to sage-duplicate/invalid/wontfix

comment:10 in reply to: ↑ 8 Changed 3 years ago by nbruin

  • 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 vbraun

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.