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 )
From this thread on sage-support:
Hi, 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/ symbolic/expression_conversions.py". 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). ?
Apply:
Attachments (4)
Change History (14)
comment:1 Changed 9 years ago by
Changed 9 years ago by
comment:3 Changed 9 years ago by
- 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
- Status changed from new to needs_review
comment:5 Changed 9 years ago by
- Status changed from needs_review to needs_work
Changed 9 years ago by
comment:6 Changed 9 years ago by
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
comment:7 Changed 9 years ago by
- 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
- 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) f(x) sage: f(x)._sympy_() <snip> 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
comment:9 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
comment:10 Changed 9 years ago by
- Merged in set to sage-5.0.beta10
- Resolution set to fixed
- Status changed from positive_review to closed
The conversions should be specified with the
conversions
argument to theBuiltinFunction
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 theconversions
argument.