translate abstract functions to Sage #13476

rwst · 2017-10-18T08:01:44Z

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 · 2017-10-28T12:56:42Z

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

rwst · 2017-11-04T15:23:22Z

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 · 2017-11-04T19:50:25Z

Please add a test to the sage tests.

asmeurer · 2017-11-04T19:51:11Z

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

rwst · 2017-11-05T06:28:33Z

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 · 2017-11-06T16:49:02Z

That should be it now.

Abdullahjavednesar · 2018-04-24T17:26:55Z

@rwst can you please resolve merge conflicts.

timokau · 2018-08-26T16:49:17Z

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

costrouc · 2019-07-20T14:52:42Z

@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 · 2019-09-22T05:46:24Z

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

sympy-bot · 2019-09-25T16:58:08Z

✅

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 · 2019-09-25T19:37:46Z

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 · 2019-09-25T20:06:23Z

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

oscarbenjamin · 2019-09-25T20:32:33Z

sympy/core/function.py

@@ -909,6 +922,7 @@ def __new__(mcl, name, bases=(AppliedUndef,), __dict__=None, **kwargs):
        __dict__['__module__'] = None
        obj = super(UndefinedFunction, mcl).__new__(mcl, name, bases, __dict__)
        obj.name = name
+        obj._sage_ = UndefSage()


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?

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?

Can we instead define a method like this?

@property def _sage_(self): ...

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

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

I meant that it's a meta class.

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

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.

Wouldn't that increase import time for sympy?

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.

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

oscarbenjamin · 2019-09-28T21:55:37Z

Looks good. Thanks.

isuruf · 2019-09-28T22:15:25Z

Thanks @oscarbenjamin for the review

translate abstract functions to Sage

2e5e40c

ylemkimon added the external label Oct 19, 2017

suppress fails from unrelated FutureWarnings

4193092

rwst mentioned this pull request Nov 4, 2017

Remove AppliedUndef.def _sage_ #11858

Closed

uncomment test which should work now

3830ab9

rwst mentioned this pull request Nov 6, 2017

move Sage conversion into Sage #13430

Closed

rwst added 2 commits November 6, 2017 16:58

add relevant test

43bc533

outcomment again the not working sympify test

866ea2d

Abdullahjavednesar added the PR: sympy's turn label Apr 24, 2018

czgdp1807 added the Could Close label Sep 22, 2019

isuruf mentioned this pull request Sep 25, 2019

Add sage conversion for UndefinedFunction #17654

Closed

Merge branch 'master' into prsage5

c6a207f

Fix bad merge

362ce30

czgdp1807 removed Could Close PR: sympy's turn labels Sep 25, 2019

Use sage.all

484b8f2

isuruf approved these changes Sep 25, 2019

View reviewed changes

oscarbenjamin reviewed Sep 25, 2019

View reviewed changes

isuruf added 2 commits September 25, 2019 15:57

Use singleton for UndefSage

14c5697

Remove use Singleton

ba81cf0

oscarbenjamin merged commit 1259825 into sympy:master Sep 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

translate abstract functions to Sage #13476

translate abstract functions to Sage #13476

rwst commented Oct 18, 2017 •

edited by isuruf

Loading

rwst commented Oct 28, 2017

rwst commented Nov 4, 2017 •

edited

Loading

asmeurer commented Nov 4, 2017

asmeurer commented Nov 4, 2017

rwst commented Nov 5, 2017

rwst commented Nov 6, 2017

Abdullahjavednesar commented Apr 24, 2018

timokau commented Aug 26, 2018

costrouc commented Jul 20, 2019

czgdp1807 commented Sep 22, 2019

sympy-bot commented Sep 25, 2019 •

edited

Loading

codecov bot commented Sep 25, 2019 •

edited

Loading

isuruf commented Sep 25, 2019 •

edited

Loading

oscarbenjamin Sep 25, 2019

isuruf Sep 25, 2019 •

edited

Loading

oscarbenjamin Sep 25, 2019

isuruf Sep 25, 2019

isuruf Sep 25, 2019

oscarbenjamin Sep 26, 2019

oscarbenjamin Sep 26, 2019

isuruf Sep 26, 2019

oscarbenjamin Sep 26, 2019

isuruf Sep 26, 2019

oscarbenjamin commented Sep 28, 2019

isuruf commented Sep 28, 2019

translate abstract functions to Sage #13476

translate abstract functions to Sage #13476

Conversation

rwst commented Oct 18, 2017 • edited by isuruf Loading

rwst commented Oct 28, 2017

rwst commented Nov 4, 2017 • edited Loading

asmeurer commented Nov 4, 2017

asmeurer commented Nov 4, 2017

rwst commented Nov 5, 2017

rwst commented Nov 6, 2017

Abdullahjavednesar commented Apr 24, 2018

timokau commented Aug 26, 2018

costrouc commented Jul 20, 2019

czgdp1807 commented Sep 22, 2019

sympy-bot commented Sep 25, 2019 • edited Loading

codecov bot commented Sep 25, 2019 • edited Loading

Codecov Report

isuruf commented Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

isuruf Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Sep 28, 2019

isuruf commented Sep 28, 2019

rwst commented Oct 18, 2017 •

edited by isuruf

Loading

rwst commented Nov 4, 2017 •

edited

Loading

sympy-bot commented Sep 25, 2019 •

edited

Loading

codecov bot commented Sep 25, 2019 •

edited

Loading

isuruf commented Sep 25, 2019 •

edited

Loading

isuruf Sep 25, 2019 •

edited

Loading