Opened 11 years ago

Closed 11 years ago

Last modified 5 years ago

#8564 closed defect (fixed)

fix atan2() conversions between Sage and SymPy

Reported by: certik Owned by: AlexGhitza
Priority: major Milestone: sage-4.5.2
Component: interfaces Keywords:
Cc: Merged in: sage-4.5.2.alpha0
Authors: Ondřej Čertík Reviewers: Burcin Erocal, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Hi,

please apply the attached patch. The corresponding test is in sympy in sympy/test_external/test_sage.py. It'd be cool to execute that file automatically in sage doctests, not sure currently how to do it.

Attachments (3)

sympy1.patch (486 bytes) - added by certik 11 years ago.
trac_8564-atan2_conversion.patch (702 bytes) - added by burcin 11 years ago.
ondrej's patch with a header
trac_8564-doctests.patch (706 bytes) - added by burcin 11 years ago.
doctest

Download all attachments as: .zip

Change History (8)

Changed 11 years ago by certik

Changed 11 years ago by burcin

ondrej's patch with a header

Changed 11 years ago by burcin

doctest

comment:1 Changed 11 years ago by burcin

  • Authors set to Ondrej Certik
  • Component changed from algebra to interfaces
  • Milestone set to sage-4.4
  • Reviewers set to Burcin Erocal
  • Status changed from new to needs_review

I uploaded two patches,

I give a positive review to Ondrej's patch, if someone can review the doctest I added this will be ready to go.

The patches to be merged are (in this order):

The doctest patch depends on #8565.

comment:2 Changed 11 years ago by kcrisman

  • Reviewers changed from Burcin Erocal to Burcin Erocal, Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

This seems fine, works well and tests the appropriate thing (i.e. not arctan2(2,3), but the symbolic thing). Positive review to both.

Question about the Sympy doctest file Ondrej mentions above - it doesn't have

check_expression("atan2(y,x)", "y x")

or whatever would work, in test_functions or something like that. Should it?

comment:3 Changed 11 years ago by certik

Thanks!

Btw, the check_expression() for atan2 is in the latest git sympy, so I need to update the Sage package for it. There were some unrelated mpmath problems with it, that I have to solve first.

Ondrej

comment:4 Changed 11 years ago by mpatel

  • Merged in set to sage-4.5.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:5 Changed 5 years ago by chapoton

  • Authors changed from Ondrej Certik to Ondřej Čertík
Note: See TracTickets for help on using tickets.