Opened 9 years ago

Closed 9 years ago

#11921 closed defect (fixed)

Allow Sympy conversion sign/sgn

Reported by: kcrisman Owned by: burcin
Priority: major Milestone: sage-5.0
Component: symbolics Keywords: sympy conversion beginner
Cc: Merged in: sage-5.0.beta10
Authors: Peter Story, Burcin Erocal Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

From this thread on sage-support:

Sage uses a dictionary to translate Sage function names to SymPy ones 
when they are different. 
Signal function is called "sgn" in Sage and "sign" in Sympy but the 
respective translation is not in the dictionary, and a error is issued 
if one tries to do something like: 
   x = var('x'); sgn(x)._sympy_() 
To fix it, it is just a matter of adding 
                "sgn": "sign", 
to the dictionary. 
Such dictionary is called "translation_table" and it is inside 
function "composition" from class "SympyConverter" of file "sage/ 
I would like to know how can I submit a patch, or ask someone to do 
it, and if it is necessary to include something in a unit test (like I 
did for Sympy for the inverse translation). 


Attachments (4)

trac_11921_conversionsDictionary.patch (708 bytes) - added by peter.story 9 years ago.
trac_11921-sympy_conversion.patch (5.2 KB) - added by burcin 9 years ago.
trac_11921-doctest.patch (654 bytes) - added by burcin 9 years ago.
trac_11921-reviewer.patch (1009 bytes) - added by kcrisman 9 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by burcin

The conversions should be specified with the conversions argument to the BuiltinFunction constructor. Having the conversion near the function definition is easier to maintain than that table.

In this case, the constructor for sage.functions.generalized.FunctionSignum? should be changed to include 'sympy': 'sign' in the conversions argument.

comment:2 Changed 9 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

Changed 9 years ago by peter.story

comment:3 Changed 9 years ago by peter.story

  • Authors set to peter.story
  • Milestone set to sage-5.0

I performed the addition to the conversions argument as requested by burcin.
However, x = var('x'); sgn(x)._sympy_() still fails.

comment:4 Changed 9 years ago by peter.story

  • Authors changed from peter.story to Peter Story
  • Status changed from new to needs_review

comment:5 Changed 9 years ago by peter.story

  • Status changed from needs_review to needs_work

Changed 9 years ago by burcin

comment:6 Changed 9 years ago by burcin

  • Authors changed from Peter Story to Peter Story, Burcin Erocal

I was wrong. For some reason, SympyConverter.composition() method defined a translation table of its own. attachment:trac_11921-sympy_conversion.patch fixes it to go through the standard mechanism. Your patch should work now.

We still need doctests to switch this to needs_review.

Changed 9 years ago by burcin

comment:7 Changed 9 years ago by burcin

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I attached a new patch that adds a doctest as well. This is ready for review now.

comment:8 Changed 9 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

I like everything here, good simplification and clarification of all this.

I'd like a different example, though!

sage: from sage.symbolic.function import SymbolicFunction 
sage: f = SymbolicFunction('f', conversions=dict(sympy='ff'))
sage: f(x)
sage: f(x)._sympy_()
ValueError: Function FallingFactorial expects 2 argument(s), got 1.

Well! That was a surprise.

So I'm adding a reviewer patch that just swaps g for f and then documents the less surprising error that sympy has no gg function.

Changed 9 years ago by kcrisman

comment:10 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta10
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.