#23496 closed defect (fixed)

sympy patch for abstract function

Reported by: mmancini Owned by: mmancini
Priority: major Milestone: sage-8.1
Component: symbolics Keywords:
Cc: tscrim, egourgoulhon, rws Merged in:
Authors: Marco Mancini Reviewers: Travis Scrimshaw, Marcelo Forets
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: bc21300 (Commits) Commit: bc21300122019b04a30dba02aaa2cd728ff7453f
Dependencies: Stopgaps:

Description

Generic function are not transformed from sympy to sage. this patch implement the PR https://github.com/sympy/sympy/pull/12826

Change History (77)

comment:1 Changed 22 months ago by mmancini

  • Branch set to public/22496_patch_sympy_abstact_function

comment:2 Changed 22 months ago by mmancini

  • Component changed from PLEASE CHANGE to symbolics
  • Owner changed from (none) to mmancini

comment:3 Changed 22 months ago by mmancini

  • Branch changed from public/22496_patch_sympy_abstact_function to public/23496_patch_sympy_abstact_function

comment:4 Changed 22 months ago by git

  • Commit set to b764ebe0dbe0fcf4f2758ca4e87bebff9116db94

Branch pushed to git repo; I updated commit sha1. New commits:

b764ebeBetter sympy patch taken from #12826 PR in sympy

comment:5 Changed 22 months ago by mmancini

  • Dependencies 22802 deleted

comment:6 Changed 22 months ago by tscrim

  • Cc tscrim added
  • Reviewers changed from tscrim to Travis Scrimshaw

Is this ready for review?

comment:7 Changed 22 months ago by mmancini

  • Status changed from new to needs_review

Yes, pleiade

comment:8 Changed 22 months ago by mmancini

  • Status changed from needs_review to needs_work

Yes, please

comment:9 Changed 22 months ago by tscrim

Is the setting this to needs_work a double-post-type error or is there an actual issue?

Also, could you ping upstream again? It would be good to know that they accepted the patch as-is.

comment:10 Changed 22 months ago by mmancini

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.
  • Status changed from needs_work to needs_review

Double error, sorry

comment:11 follow-up: Changed 22 months ago by tscrim

You need to also bump the patch level in package-version.txt so that it rebuilds sympy.

Did you send another message to upstream asking about the status of the patch?

comment:12 Changed 22 months ago by git

  • Commit changed from b764ebe0dbe0fcf4f2758ca4e87bebff9116db94 to 9d16a70b2daf9e123f2bf180547f38a4a10c2306

Branch pushed to git repo; I updated commit sha1. New commits:

9d16a70Lost package-version.txt modifications : restored

comment:13 Changed 22 months ago by git

  • Commit changed from 9d16a70b2daf9e123f2bf180547f38a4a10c2306 to d5d6b6733ea408b4a73860578e8b482d6dfea2c7

Branch pushed to git repo; I updated commit sha1. New commits:

d5d6b67Corrected error on patcj version

comment:14 Changed 22 months ago by git

  • Commit changed from d5d6b6733ea408b4a73860578e8b482d6dfea2c7 to 462bd681d32ee87a3813caa79e43842a4477d2e7

Branch pushed to git repo; I updated commit sha1. New commits:

462bd68test is not corrected at this point

comment:15 in reply to: ↑ 11 Changed 22 months ago by mmancini

Replying to tscrim:

You need to also bump the patch level in package-version.txt so that it rebuilds sympy.

Did you send another message to upstream asking about the status of the patch?

The referee said me that tests are still falling on travis. I asked to see the report (I have not access to travis).

comment:16 Changed 22 months ago by mmancini

Last edited 22 months ago by mmancini (previous) (diff)

comment:17 Changed 22 months ago by mmancini

I have submitted the correction to the sympy ticket, If it passes the tests then I will change it also in the current branch

comment:18 Changed 22 months ago by git

  • Commit changed from 462bd681d32ee87a3813caa79e43842a4477d2e7 to daa2f2cc0a70480553db6a01fd9b28552e63920e

Branch pushed to git repo; I updated commit sha1. New commits:

daa2f2cError on sympy branch corrected with a new patch

comment:19 Changed 22 months ago by git

  • Commit changed from daa2f2cc0a70480553db6a01fd9b28552e63920e to 487242514e520b0c73e24628ac2269df7cd2c0c0

Branch pushed to git repo; I updated commit sha1. New commits:

4872425forgetten to delete gg in doctest

comment:20 Changed 22 months ago by mmancini

The sympy patch was merged 12826

comment:21 Changed 22 months ago by tscrim

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

Just put your real names in the author field, bump the patch level in sympy, and remove the trivial whitespace change in function.pyx. Once done, you can set a positive review on my behalf.

comment:22 Changed 22 months ago by git

  • Commit changed from 487242514e520b0c73e24628ac2269df7cd2c0c0 to 721eed7a72e7d86189fa6200bed75ef1c11af4a3

Branch pushed to git repo; I updated commit sha1. New commits:

721eed7removed whitespace

comment:23 Changed 22 months ago by mmancini

  • Authors changed from mmancini, egourgoulhon to Marco Mancini
  • Status changed from needs_review to positive_review

comment:24 Changed 22 months ago by mmancini

I hope it is ok : the function.pyx is now at the old state, i reversed it to before my modifications.

comment:25 Changed 22 months ago by tscrim

  • Milestone changed from sage-8.0 to sage-8.1
  • Status changed from positive_review to needs_work

You still need to bump the patch level:

-1.0.p2
+1.0.p3

in $SAGE_ROOT/build/pkgs/sympy/package-version.txt.

comment:26 Changed 22 months ago by git

  • Commit changed from 721eed7a72e7d86189fa6200bed75ef1c11af4a3 to e46345b0c23138d017ce6f50f44d144179c9c668

Branch pushed to git repo; I updated commit sha1. New commits:

e46345bchanged sympy patch version

comment:27 Changed 22 months ago by git

  • Commit changed from e46345b0c23138d017ce6f50f44d144179c9c668 to 46ebd728aac486a5dd675f5d228d90001c421635

Branch pushed to git repo; I updated commit sha1. New commits:

46ebd72Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function

comment:28 Changed 22 months ago by tscrim

  • Status changed from needs_work to positive_review

Trivial rebase with 8.1beta0. Thanks, LGTM.

comment:29 Changed 22 months ago by vbraun

  • Status changed from positive_review to needs_work

See patchbot

comment:30 Changed 22 months ago by fbissey

This is quite interesting as failure

sage -t --long /usr/lib64/python2.7/site-packages/sage/tests/french_book/recequadiff.py
too many failed tests, not using stored timings
Running doctests with ID 2017-08-01-11-17-21-10bca036.
Using --optional=optional,sage
Doctesting 1 file.
sage -t --long /usr/lib64/python2.7/site-packages/sage/tests/french_book/recequadiff.py
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/tests/french_book/recequadiff.py", line 383, in sage.tests.french_book.recequadiff
Failed example:
    f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 512, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 875, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.french_book.recequadiff[108]>", line 1, in <module>
        f = u(n+Integer(2))-(Integer(3)/Integer(2))*u(n+Integer(1))+(Integer(1)/Integer(2))*u(n)
      File "/usr/lib64/python2.7/site-packages/sympy/core/decorators.py", line 76, in __sympifyit_wrapper
        b = sympify(b, strict=True)
      File "/usr/lib64/python2.7/site-packages/sympy/core/sympify.py", line 265, in sympify
        return a._sympy_()
      File "sage/symbolic/expression.pyx", line 1445, in sage.symbolic.expression.Expression._sympy_ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/expression.cpp:11783)
        return sympy(self)
      File "/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 218, in __call__
        return self.arithmetic(ex, operator)
      File "/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 716, in arithmetic
        ops = [sympy.sympify(self(a), evaluate=False) for a in ex.operands()]
      File "/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 226, in __call__
        return self.composition(ex, operator)
      File "/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 767, in composition
        raise NotImplementedError("SymPy function '%s' doesn't exist" % f)
    NotImplementedError: SymPy function 'u' doesn't exist
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/tests/french_book/recequadiff.py", line 388, in sage.tests.french_book.recequadiff
Failed example:
    rsolve(f, u(n), {u(0):-1,u(1):1})
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 512, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 875, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.french_book.recequadiff[110]>", line 1, in <module>
        rsolve(f, u(n), {u(Integer(0)):-Integer(1),u(Integer(1)):Integer(1)})
      File "/usr/lib64/python2.7/site-packages/sympy/solvers/recurr.py", line 722, in rsolve
        f = f.expand().collect(y.func(Wild('m', integer=True)))
      File "sage/symbolic/expression.pyx", line 7163, in sage.symbolic.expression.Expression.collect (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/expression.cpp:42956)
        cdef Expression s0 = self.coerce_in(s)
      File "sage/symbolic/expression.pyx", line 2956, in sage.symbolic.expression.Expression.coerce_in (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/expression.cpp:22665)
        return self._parent._coerce_(z)
      File "sage/structure/parent_old.pyx", line 230, in sage.structure.parent_old.Parent._coerce_ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/parent_old.c:4888)
        return self.coerce(x)
      File "sage/structure/parent.pyx", line 1168, in sage.structure.parent.Parent.coerce (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/parent.c:11000)
        mor = self._internal_coerce_map_from(parent(x))
      File "sage/structure/parent.pyx", line 2107, in sage.structure.parent.Parent._internal_coerce_map_from (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/parent.c:18735)
        mor = self.discover_coerce_map_from(S)
      File "sage/structure/parent.pyx", line 2246, in sage.structure.parent.Parent.discover_coerce_map_from (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/structure/parent.c:19211)
        user_provided_mor = self._coerce_map_from_(S)
      File "sage/symbolic/ring.pyx", line 91, in sage.symbolic.ring.SymbolicRing._coerce_map_from_ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/ring.cpp:4056)
        cpdef _coerce_map_from_(self, R):
      File "/usr/lib64/python2.7/site-packages/sage/symbolic/callable.py", line 310, in _coerce_map_from_
        return SymbolicRing._coerce_map_from_(self, R)
      File "sage/symbolic/ring.pyx", line 91, in sage.symbolic.ring.SymbolicRing._coerce_map_from_ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/ring.cpp:5643)
        cpdef _coerce_map_from_(self, R):
      File "sage/symbolic/ring.pyx", line 167, in sage.symbolic.ring.SymbolicRing._coerce_map_from_ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/ring.cpp:4315)
        if 'sympy' in R.__module__:
    TypeError: argument of type 'NoneType' is not iterable
**********************************************************************
1 item had failures:
   2 of 115 in sage.tests.french_book.recequadiff
    [114 tests, 2 failures, 4.43 s]
----------------------------------------------------------------------
sage -t --long /usr/lib64/python2.7/site-packages/sage/tests/french_book/recequadiff.py  # 2 doctests failed

The first doctest goes

Python 2.7.12 (default, Feb 16 2017, 16:28:54) 
[GCC 4.9.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from sympy import Function, Symbol
>>> u = Function('u'); n = Symbol('n', integer=True)
>>> f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)
>>>

and is no problem in python. But in sage "boom". But there is more

sage: from sympy import Function, Symbol
sage: u = Function('u'); n = Symbol('n', integer=True)
sage: f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-3-a0e08a1755b9> in <module>()
----> 1 f = u(n+Integer(2))-(Integer(3)/Integer(2))*u(n+Integer(1))+(Integer(1)/Integer(2))*u(n)

/usr/lib64/python2.7/site-packages/sympy/core/decorators.pyc in __sympifyit_wrapper(a, b)
     74                 # with sympy objects. Otherwise, it must be converted.
     75                 if not hasattr(b, '_op_priority'):
---> 76                     b = sympify(b, strict=True)
     77                 return func(a, b)
     78             except SympifyError:

/usr/lib64/python2.7/site-packages/sympy/core/sympify.pyc in sympify(a, locals, convert_xor, strict, rational, evaluate)
    263 
    264     try:
--> 265         return a._sympy_()
    266     except AttributeError:
    267         pass

/usr/lib64/python2.7/site-packages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._sympy_ (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/symbolic/expression.cpp:11783)()
   1443         """
   1444         from sage.symbolic.expression_conversions import sympy
-> 1445         return sympy(self)
   1446 
   1447     def _algebraic_(self, field):

/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.pyc in __call__(self, ex)
    216                 div = self.get_fake_div(ex)
    217                 return self.arithmetic(div, div.operator())
--> 218             return self.arithmetic(ex, operator)
    219         elif operator in relation_operators:
    220             return self.relation(ex, operator)

/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.pyc in arithmetic(self, ex, operator)
    714         import sympy
    715         operator = arithmetic_operators[operator]
--> 716         ops = [sympy.sympify(self(a), evaluate=False) for a in ex.operands()]
    717         if operator == "+":
    718             return sympy.Add(*ops)

/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.pyc in __call__(self, ex)
    224             return self.tuple(ex)
    225         else:
--> 226             return self.composition(ex, operator)
    227 
    228     def get_fake_div(self, ex):

/usr/lib64/python2.7/site-packages/sage/symbolic/expression_conversions.pyc in composition(self, ex, operator)
    765             return f_sympy(*sympy.sympify(g, evaluate=False))
    766         else:
--> 767             raise NotImplementedError("SymPy function '%s' doesn't exist" % f)
    768 
    769 sympy = SympyConverter()

NotImplementedError: SymPy function 'u' doesn't exist
sage: f = 1*u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)
sage: 

That's right, multiply the first term by 1 the problem disappear.

comment:31 Changed 22 months ago by mmancini

What is sage "boom" ? the problem was not present before the rebase with 8.1beta0, is it true?

comment:32 Changed 22 months ago by fbissey

Pardon me my antics. "boom" as in sage breaks down. Just a minor explosion since it doesn't crash, just tells you off. I don't know if it was present before 8.1beta0. My only suggestion on a possible cause is #23413 but that's just a stab in the dark.

comment:33 Changed 22 months ago by mmancini

Effectively the error disappear when I revert the changes in sympy. but it before the 8.1beta0 it was ok.

comment:34 follow-up: Changed 21 months ago by mforets

the missing conversion is the composition of functions in the sympy converter, which was done here: #20204.

comment:35 in reply to: ↑ 34 ; follow-up: Changed 21 months ago by mforets

Replying to mforets:

the missing conversion is the composition of functions in the sympy converter, which was done here: #20204.

Shall the patch be added here, or you prefer to create a dependency for this one?

(FYI: the problem with #20204 is that i couldn't patch sympy such that f._sage_() works, so that we have conversions in both ways, as commented in the sympy PR.)

comment:36 Changed 21 months ago by egourgoulhon

  • Cc egourgoulhon added

comment:37 in reply to: ↑ 35 Changed 21 months ago by mmancini

I think that you should create a dependency on this ticket in your (#20204). This ticket was created to separate the sympy patch from the modifications done in Sage. This ticket is yet a dependency of #22802

Replying to mforets:

Replying to mforets:

the missing conversion is the composition of functions in the sympy converter, which was done here: #20204.

Shall the patch be added here, or you prefer to create a dependency for this one?

(FYI: the problem with #20204 is that i couldn't patch sympy such that f._sage_() works, so that we have conversions in both ways, as commented in the sympy PR.)

comment:38 Changed 21 months ago by mforets

  • Status changed from needs_work to needs_info

OK, done. if you pull #20204 which has been merged with this one, there is no "boom".

but we haven't changed anything in this one, so how is it possible that this ticket gets in?

OTOH, I'm afraid there will be conflicts with #22802.

comment:39 follow-up: Changed 21 months ago by mmancini

The boom was caused by the rebase with 8.10beta0. has your branch done this operation? I think no. Before the rebase there was not error in the tests.

comment:40 in reply to: ↑ 39 Changed 21 months ago by mforets

Replying to mmancini:

The boom was caused by the rebase with 8.10beta0. has your branch done this operation?

the branch t/mforets/20204 has been merged with 8.1.beta3.

comment:41 Changed 21 months ago by fbissey

If you merged your stuff with #20204 and it cannot work without the rest of #20204 we have two solutions:

  • make the tickets depend on each other. Feels circular but it is an indication that the tickets need to be merged together.
  • mark this ticket as a duplicate and close it, leaving only the branch at #20204.

Bonus question: Will we be able to upgrade sympy to 1.1.1 easily once everything is in? Currently it leads to failures, again because of missing converter if I am not mistaken (this is a report from sage-on-gentoo don't look for an upgrade ticket at this stage).

comment:42 Changed 21 months ago by mmancini

This ticket was part of the #22802 (now #22802 ticket depends on this ticket). The referee asked to separate it in 2 tickets : #22802 containing the converter ._sympy_() and the #23496 containing only the sympy patch (._sage())

comment:43 Changed 21 months ago by mforets

Ok, then i upvote for making this and #20204 depending on each other. afterwards, merge with #22802.

comment:44 Changed 21 months ago by git

  • Commit changed from 46ebd728aac486a5dd675f5d228d90001c421635 to 67f7f4693f271e068e0e073b70909d5013608cf0

Branch pushed to git repo; I updated commit sha1. New commits:

67f7f46integrated modifications to composition. But an error remains in french_book.recequadiff tests

comment:45 Changed 21 months ago by git

  • Commit changed from 67f7f4693f271e068e0e073b70909d5013608cf0 to 1b5227d6a85a540663aa138e8304a9055ba110fd

Branch pushed to git repo; I updated commit sha1. New commits:

1b5227dError in french_book.recequadiff tests corrected

comment:46 follow-up: Changed 21 months ago by mforets

@mmancini : i don't understand the last two commits. those changes are changing the purpose of this ticket. what do you think about comment:43?

comment:47 in reply to: ↑ 46 Changed 21 months ago by mmancini

Replying to mforets:

@mmancini : i don't understand the last two commits. those changes are changing the purpose of this ticket. what do you think about comment:43?

After a week of debugging I think that this is the unique to pass the tests in french_book.recequadiff : in test : 383 sage: f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n) the integer 3/2 (Sage Integer) makes necessary the modifications (you suggested) to composition function: u(n+2) is sympy (3/2)*u(n+1) is sage ... ...

the error in 388 sage: rsolve(f, u(n), {u(0):-1,u(1):1}) is caused by the fact that Sympy integer are not imported.

Before the sympy patch there was not error but for me it was not correct: because the expressions mixed sympy (u,n) and sage quantities (1/2).

comment:48 Changed 21 months ago by tscrim

That is quite a piece of debugging. However, there should be some natural place to put in this import statement within the code, either in Sage or Sympy. Do you have an idea about where it should go?

comment:49 Changed 21 months ago by mmancini

I have no idea. For me the expression is not well written: if I am using sage and I want resolve it with a sympy method: before I write the expression than I covert it before to use sympy method and finally I convert back the result. But a strange mixing for me is not sane.

comment:50 follow-up: Changed 21 months ago by mforets

what i find odd is that:

  • using n = Symbol('n') in the rsolve test it works,
  • similarly but using from sympy.abc import n.

to move on, i propose to modify the test as below, and report the failing use case in a new ticket.

sage: n = var('n', domain='integer'); u = function('u')
sage: f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)
sage: us = u._sympy_() ; fs = f._sympy_();
sage: from sympy import rsolve; rsolve(fs, us(n), {us(0):-1,us(1):1})
3 - 4*2**(-n)

what do you think?

comment:51 follow-up: Changed 21 months ago by mforets

  • also works if you cast the sage integers to symbolic expressions, the rest of the test unchanged: f = u(n+2)-SR(3/2)*u(n+1)+SR(1/2)*u(n)

comment:52 in reply to: ↑ 50 ; follow-up: Changed 21 months ago by mmancini

I agree,It is a coherent way to do things. We need to do another ticket?

Replying to mforets:

what i find odd is that:

  • using n = Symbol('n') in the rsolve test it works,
  • similarly but using from sympy.abc import n.

to move on, i propose to modify the test as below, and report the failing use case in a new ticket.

sage: n = var('n', domain='integer'); u = function('u')
sage: f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)
sage: us = u._sympy_() ; fs = f._sympy_();
sage: from sympy import rsolve; rsolve(fs, us(n), {us(0):-1,us(1):1})
3 - 4*2**(-n)

what do you think?

comment:53 in reply to: ↑ 51 Changed 21 months ago by mmancini

Replying to mforets:

  • also works if you cast the sage integers to symbolic expressions, the rest of the test unchanged: f = u(n+2)-SR(3/2)*u(n+1)+SR(1/2)*u(n)

It is also possible to postpone rational numbers to prevent sympy-two-day intermediate conversion :f = u(n+2)-u(n+1)*3/2+u(n)*1/2

comment:54 in reply to: ↑ 52 ; follow-up: Changed 21 months ago by egourgoulhon

Replying to mmancini:

I agree,It is a coherent way to do things.

+1

We need to do another ticket?

IMHO yes, because this is a different issue; the new ticket could be about implementing rsolve in Sage, as an interface to sympy's rsolve.

The doctest discussed here is used as an example on page 17 of this document: https://members.loria.fr/PZimmermann/sagebook/recequadiff.pdf , which is a chapter of the English translation of the so-called "French book" (Calcul mathématique avec Sage). The latter is currently in its final preparatory stage, cf. https://groups.google.com/forum/#!topic/sage-devel/MalKzTyvrys . One should probably ask the authors to update this example (the book is not likely to be out before Sage 8.1, where the current ticket could be effective).

comment:55 Changed 21 months ago by tscrim

  • Status changed from needs_info to needs_work

I understand what changes with that import statement, every Integer becomes a Sympy Integer because the preparser replaces things like 1 with Integer(1). I strongly believe we should be testing behavior that mixes Sage and Sympy, as this is a very natural thing to do (in Sage), so I disagree with changing the test and also 1b5227d.

comment:56 Changed 21 months ago by tscrim

Interesting, this works on my machine:

f = u(n+2)-u(n+1)*(3/2)+u(n)/2

Doing more debugging currently.

comment:57 Changed 21 months ago by tscrim

It seems to boil back down to this:

sage: type((3/2) * u(n+1))
<type 'sage.symbolic.expression.Expression'>
sage: type(u(n+1) * (3/2))
<class 'sympy.core.mul.Mul'>

comment:58 Changed 21 months ago by tscrim

Actually, that might just be a symptom rather than the cause. I'm wondering if there is something wrong with doing the conversion to Sympy:

sage: f = u(n+2)-sympy.sympify(3/2)*u(n+1)+sympy.sympify(1/2)*u(n)  # This works
sage: f = u(n+2)-sympy.sympify((3/2)*u(n+1))+sympy.sympify((1/2)*u(n))  # This fails

comment:59 Changed 21 months ago by tscrim

  • Cc rws added

I think this is the real problem with all of this:

sage: from sympy import Symbol
sage: n = Symbol('n', integer=True)
sage: sympy.sympify(1+n).args[1] == n
False

The roundtrip of a sympy symbol through Sage results in a different symbol:

sage: m = Symbol('n', integer=True)
sage: m == n
True
sage: p = Symbol('n')
sage: p == n
False

sage: sympy.simplify(SR(n)).assumptions0
{'commutative': True}
sage: n.assumptions0
{'algebraic': True,
 'commutative': True,
 'complex': True,
 'hermitian': True,
 'imaginary': False,
 'integer': True,
 'irrational': False,
 'noninteger': False,
 'rational': True,
 'real': True,
 'transcendental': False}
sage: p is sympy.sympify(SR(n))
True

So I think we need to translate assumptions over to truly fix the problem.

comment:60 follow-up: Changed 21 months ago by tscrim

I'm also a little more open to just fixing the test (but making sure everything says in Sympy with a big warning message in the doctest).

comment:61 follow-up: Changed 21 months ago by tscrim

  • Status changed from needs_work to needs_info

I guess what I am asking is do we want to fix the problem of comment:59 here or just push it to a later ticket?

comment:62 in reply to: ↑ 60 ; follow-up: Changed 21 months ago by egourgoulhon

Replying to tscrim:

I'm also a little more open to just fixing the test

Indeed, the test, as it is, is quite unnatural from a Sage's user point of view: the import of specific Sympy objects (in the line from sympy import Function, Symbol) and their mixing with some Sage objects (in the line f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)), with *implicit* conversions Sage <--> Sympy involved, without any user control on them, is not something desirable. The change proposed in comment:50 seems much more reasonable: one defines first f entirely as a Sage object and then *explicitely* convert it to a Sympy object before passing it to Sympy's rsolve, because there is no Sage's rsolve yet.

comment:63 in reply to: ↑ 61 Changed 21 months ago by egourgoulhon

Replying to tscrim:

I guess what I am asking is do we want to fix the problem of comment:59 here or just push it to a later ticket?

+1 for a later ticket.

comment:64 in reply to: ↑ 62 ; follow-up: Changed 21 months ago by tscrim

Replying to egourgoulhon:

Replying to tscrim:

I'm also a little more open to just fixing the test

Indeed, the test, as it is, is quite unnatural from a Sage's user point of view: the import of specific Sympy objects (in the line from sympy import Function, Symbol) and their mixing with some Sage objects (in the line f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)), with *implicit* conversions Sage <--> Sympy involved, without any user control on them, is not something desirable. The change proposed in comment:50 seems much more reasonable: one defines first f entirely as a Sage object and then *explicitely* convert it to a Sympy object before passing it to Sympy's rsolve, because there is no Sage's rsolve yet.

I don't think it is that unnatural as someone who wants to use sympy, something that we do suggest to people, would construct something in that fashion. One of the benefits of Sage are those implicit conversions so that users should not have to worry about them.

However, that discussion aside, I think that we should keep as close to the original doctest because it is in Calcul Mathématique avec Sage (even if it is out of date). So I believe we should just change the order of multiplication of the coefficients as in comment:56 and add a warning about it.

comment:65 Changed 21 months ago by mforets

hi, thank you all for the help debugging. with this sympy patch (at sympy/core/function.py and on top of #20204), it works for me:

class AppliedUndef(Function):
    """
    Base class for expressions resulting from the application of an undefined
    function.
    """
    .
    .
    .
    def _sage_(self):
        import sage.all as sage
        fname = self.func.__name__
        func = getattr(sage, fname)
        args = [arg._sage_() for arg in self.args]
        return func

do you already have the same _sage_ there?

comment:66 in reply to: ↑ 64 Changed 21 months ago by egourgoulhon

Replying to tscrim:

I don't think it is that unnatural as someone who wants to use sympy, something that we do suggest to people, would construct something in that fashion. One of the benefits of Sage are those implicit conversions so that users should not have to worry about them.

Well, there is some degree of arbitrariness in these implicit conversions: if a is a Sage object and b a Sympy object, what a*b shall be? For the example of the French textbook as it is now, with a=3/2 and b=u(n+1), we want a*b to be a Sympy object, so that the final f is a Sympy object. But in other cases?

comment:67 Changed 21 months ago by mforets

So I believe we should just change the order of multiplication of the coefficients as in comment:56 and add a warning about it.

it seems that game was played before:

$ git show 12cdb58d7a02391224477d0d99312be8fc03f678
@@ -379,7 +379,7 @@ Sage example in ./recequadiff.tex, line 1197::

 Sage example in ./recequadiff.tex, line 1208::


-  sage: f = u(n+2)-u(n+1)*(3/2)+u(n)*(1/2)
+  sage: f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)

this is a commit by Ralph.

comment:68 Changed 21 months ago by tscrim

@mforets comment:67 - All the more reason to just do the minimal change. IIRC, we needed to do that in order for the construction of f to not fail, but with this patch, it is okay.

@egourgoulhon comment:66 - Yes, there is currently some ambiguity because of things are done, but ultimately it shouldn't matter. Although if I really had to decide, I would say everything should go to Sympy objects because we cannot expect Sympy objects to work with our coercion system.

comment:69 follow-up: Changed 21 months ago by mforets

  • Branch changed from public/23496_patch_sympy_abstact_function to public/23496_patch_sympy_abstract_function
  • Commit changed from 1b5227d6a85a540663aa138e8304a9055ba110fd to d76caea0b97af8769edf9a1064db9f7486232b1d
  • Status changed from needs_info to needs_review

ok, let's do it => needs review

note: this does not depend on #20204.

(@Marco : sorry, i did not intend to create a new branch, just after pushing i realized the current says "abstact" instead of "abstract")


New commits:

018d753Merge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function
94af74b#23496 : (temp) revert order in rsolve recequadiff.tex test
d76caea#23496 : add message for change in doctest

comment:70 Changed 21 months ago by git

  • Commit changed from d76caea0b97af8769edf9a1064db9f7486232b1d to bc21300122019b04a30dba02aaa2cd728ff7453f

Branch pushed to git repo; I updated commit sha1. New commits:

bc21300#23496 : fix a _sympy_init_ doctest

comment:71 in reply to: ↑ 54 Changed 21 months ago by mforets

We need to do another ticket?

IMHO yes, because this is a different issue; the new ticket could be about implementing rsolve in Sage, as an interface to sympy's rsolve.

yes, that would be a nice feature. we can use ticket #1291. also open is the sage interface to sympy's solve (#22322).

comment:72 in reply to: ↑ 69 Changed 21 months ago by mmancini

Replying to mforets:

ok, let's do it => needs review

note: this does not depend on #20204.

(@Marco : sorry, i did not intend to create a new branch, just after pushing i realized the current says "abstact" instead of "abstract")

No problem, thanks.

comment:73 Changed 21 months ago by mforets

the patchbot errrors seem to be unrelated to our patch, but on a GLPK update.

comment:74 Changed 21 months ago by tscrim

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Marcelo Forets
  • Status changed from needs_review to positive_review

Positive review. Thanks for your work on this.

comment:75 Changed 21 months ago by rws

Thanks from me as well.

comment:76 Changed 21 months ago by mmancini

Thank you for the review

comment:77 Changed 21 months ago by vbraun

  • Branch changed from public/23496_patch_sympy_abstract_function to bc21300122019b04a30dba02aaa2cd728ff7453f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.