translate abstract functions to Sage

[rwst]

This finally fixes the conversion of abstract functions to Sage. Reason to do it this way:

When F=Function('abc') is called the constructor of Function uses the metaclass UndefFunction to create a new class with parent AppliedUndef named abc. Calling F._sage_() will NOT call the _sage_() member of AppliedUndef but tries to call abc._sage_() which does not exist at installation time. This, however, is not a problem because we can dynamically add a _sage_() method to abc on creation of abc inside UndefFunction.__new__. So converting abstract functions works.

The problem is that now F(x)._sage_() will call the same abc._sage_() class method while previously it was referred to the superclass, i.e. the AppliedUndef._sage_() member function. But _sage_ as class method does not know about the specific instance and its argument x!

This issue does not contain test code because testing Sage in Travis does not work at the moment and the PR patch is tested in Sage thoroughly, see https://trac.sagemath.org/ticket/20204

• functions
  • Fixed conversion of UndefinedFunction to sage

[rwst]

No travis log. I restarted the build but it doesn't show here.

[rwst]

The plotting tests fail under Py3 with:

______________ sympy/plotting/tests/test_plot.py:test_matplotlib _______________
  File "/usr/lib/python3.4/site-packages/sympy-1.1.2.dev0-py3.4.egg/sympy/plotting/tests/test_plot.py", line 262, in test_matplotlib
    plot_and_save('test')
  File "/usr/lib/python3.4/site-packages/sympy-1.1.2.dev0-py3.4.egg/sympy/plotting/tests/test_plot.py", line 229, in plot_and_save
    assert issubclass(i.category, UserWarning)
AssertionError

The reason is the warning matplotlib/collections.py:590: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison and the failed test is triggered because this warning is unexpected.

https://stackoverflow.com/questions/40659212/futurewarning-elementwise-comparison-failed-returning-scalar-but-in-the-futur

[asmeurer]

Please add a test to the sage tests.

[asmeurer]

Note to whomever merges this: the sage tests are set to allowed failure in Travis, so you need to check them manually.

[rwst]

See also https://trac.sagemath.org/ticket/20204
which adds and tests this patch with SymPy-1.0,
and https://trac.sagemath.org/ticket/24062
where SymPy is upgraded to 1.1.1 with the patch included.
Note that 4193092 is not included because we didn't have that failure on Sage (which uses Python2).

[rwst]

That should be it now.

[Abdullahjavednesar]

@rwst can you please resolve merge conflicts.

[timokau]

@rwst are you still interested in this? 03_undeffun_sage.patch is the only remaining patch that sage applies to sympy.

[costrouc]

@timokau this patch to sympy https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/sympy/default.nix#L25 in nix break with current master. @rwst still interested in this PR?

[czgdp1807]

@rswt Are you still interested to work on this PR?

[sympy-bot]

✅

Hi, I am the SymPy bot (v149). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• functions
  • Fixed conversion of UndefinedFunction to sage (#13476 by @isuruf and @rwst)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

This finally fixes the conversion of abstract functions to Sage. Reason to do it this way:

When `F=Function('abc')` is called the constructor of `Function` uses the metaclass `UndefFunction` to create a new class with parent `AppliedUndef` named `abc`. Calling `F._sage_()` will NOT call the `_sage_()` member of `AppliedUndef` but tries to call `abc._sage_()` which does not exist at installation time. This, however, is not a problem because we can dynamically add a `_sage_()` method to `abc` on creation of `abc` inside `UndefFunction.__new__`. So converting abstract functions works.

The problem is that now `F(x)._sage_()` will call the same `abc._sage_()` class method while previously it was referred to the superclass, i.e. the `AppliedUndef._sage_()` member function. But `_sage_` as class method does not know about the specific instance and its argument `x`!

This issue does not contain test code because testing Sage in Travis does not work at the moment and the PR patch is tested in Sage thoroughly, see https://trac.sagemath.org/ticket/20204

<!-- BEGIN RELEASE NOTES -->
* functions
  * Fixed conversion of UndefinedFunction to sage
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[codecov]

Codecov Report

Merging #13476 into master will decrease coverage by 12.796%.
The diff coverage is 44.444%.

@@              Coverage Diff              @@
##           master    #13476        +/-   ##
=============================================
- Coverage   74.76%   61.964%   -12.797%     
=============================================
  Files         634       634                
  Lines      165333    165342         +9     
  Branches    38857     38861         +4     
=============================================
- Hits       123604    102453     -21151     
- Misses      36309     57050     +20741     
- Partials     5420      5839       +419

[isuruf]

Travis-CI build with sage doesn't produce coverage reports, so the lines in coverage report are red, but those lines are tested.

[oscarbenjamin]

Does this need to be a different instance for each undefined function? As far as I can tell the UndefSage class has no instance data so it can be a singleton class. In fact why isn't it just a property-method using @property?

[isuruf]

I changed it to a Singleton class.

In fact why isn't it just a property-method using @property?

What do you mean by this?

[oscarbenjamin]

Can we instead define a method like this?

    @property
    def _sage_(self):
        ...

It isn't normally necessary to explicitly create a descriptor object.

[isuruf]

No, this is a class with AppliedUndef as a base and therefore its _sage_ is called instead of the method like you mentioned.

[isuruf]

I meant that it's a meta class.

[oscarbenjamin]

It does seem that way (I'm trying to think of an alternative...)

[oscarbenjamin]

Btw when I said the class could be a singleton I just meant that it could be like:

class A:
    pass
a = A()
...
obj._sage_ = a

rather than using more metaclasses.

[isuruf]

Wouldn't that increase import time for sympy?

[oscarbenjamin]

I don't see why it would increase import time. Here's a quick test (module t.py):

from sympy.core.compatibility import with_metaclass
from sympy.core.singleton import Singleton

def f():
    class A(with_metaclass(Singleton)):
        pass

def g():
    class B:
        pass
    b = B()

Running this with timeit I get:

$ python -m timeit -s 'from t import f, g' 'f()'
1000 loops, best of 5: 222 usec per loop
$ python -m timeit -s 'from t import f, g' 'g()'
50000 loops, best of 5: 9.72 usec per loop

So normal Python classes are 20 times faster than Singleton/metaclass.

The purpose of the Singleton metaclass is to install the class in the singleton registry so it becomes available as e.g. S.Infinity which isn't what we want here.

[isuruf]

Thanks @oscarbenjamin, I've removed the use of Singleton metaclass and used the method you proposed.

[oscarbenjamin]

Looks good. Thanks.

[isuruf]

Thanks @oscarbenjamin for the review