Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#1382 closed defect (fixed)

[with patch, with positive review] conversion of sage matrices to mathematica is just completely totally broken

Reported by: was Owned by: TimothyClemans
Priority: major Milestone: sage-2.10.3
Component: interfaces Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

We have

sage: n = matrix(QQ, 3, range(9))
sage: n._mathematica_init_()
'{{0},{1},{2},{3},{4},{5},{6},{7},{8}}'

but we should have

sage: n = matrix(QQ, 3, range(9))
sage: n._mathematica_init_()
'{{0,1,2},{3,4,5},{6,7,8}}'

Attachments (7)

ticket_1382.patch (1.5 KB) - added by TimothyClemans 10 years ago.
task_1832_2.patch (1.4 KB) - added by TimothyClemans 10 years ago.
task_1832_3.patch (982 bytes) - added by TimothyClemans 10 years ago.
task_1382_4.patch (1.6 KB) - added by TimothyClemans 10 years ago.
combined_1382.patch (1.6 KB) - added by TimothyClemans 10 years ago.
1382_5.patch (2.1 KB) - added by TimothyClemans 10 years ago.
1382_5-part2of2.patch (2.7 KB) - added by was 10 years ago.
(this one by William and Timothy) apply this patch right after applying 1382_2.patch

Download all attachments as: .zip

Change History (21)

comment:1 Changed 10 years ago by mabshoff

  • Milestone changed from sage-2.10 to sage-2.9.1

Changed 10 years ago by TimothyClemans

comment:2 Changed 10 years ago by mabshoff

  • Summary changed from conversion of sage matrices to mathematica is just completely totally broken to [with patch, needs review] conversion of sage matrices to mathematica is just completely totally broken

Changed 10 years ago by TimothyClemans

Changed 10 years ago by TimothyClemans

Changed 10 years ago by TimothyClemans

comment:3 Changed 10 years ago by TimothyClemans

  • Owner changed from was to TimothyClemans
  • Status changed from new to assigned

Changed 10 years ago by TimothyClemans

comment:4 Changed 10 years ago by ncalexan

  • Summary changed from [with patch, needs review] conversion of sage matrices to mathematica is just completely totally broken to [with patch, with negative review] conversion of sage matrices to mathematica is just completely totally broken

This will not work, since it does not appear to be recursive. For example:

sage: var('x, y, z, b')
(x, y, z, b)
sage: f = sin(x^2) + y^z
sage: f
y^z + sin(x^2)
sage: f._mathematica_init_()
'(Sin[(x) ^ (2)]) + ((y) ^ (z))'
sage: M = matrix(1, 2, [f, f^2]); M
[    y^z + sin(x^2) (y^z + sin(x^2))^2]

Also, please post a unified patch making it easy to see just the total changes.

Changed 10 years ago by TimothyClemans

Changed 10 years ago by was

(this one by William and Timothy) apply this patch right after applying 1382_2.patch

comment:5 Changed 10 years ago by was

  • Summary changed from [with patch, with negative review] conversion of sage matrices to mathematica is just completely totally broken to [with patch, needs new review] conversion of sage matrices to mathematica is just completely totally broken

comment:6 Changed 10 years ago by ncalexan

  • Summary changed from [with patch, needs new review] conversion of sage matrices to mathematica is just completely totally broken to [with patch, with positive review] conversion of sage matrices to mathematica is just completely totally broken

Patch looks good, I say apply.

Is _mathematica_init_ guaranteed not to require Mathematica? If not, some more doctests need to be declared optional. Otherwise, looks good.

comment:7 Changed 10 years ago by was

Is _mathematica_init_ guaranteed not to require Mathematica?

Yes. It returns a string is must not call Mathematica.

William

comment:8 Changed 10 years ago by mabshoff

Somebody ought to clue be in

  • which patches to apply in which order (the comments are unclear)
  • if all problems regarding optional doctests are solved, i.e. the last comment by William

Cheers,

Michael

comment:9 Changed 10 years ago by was

Michael. Apply 1382_2.patch then 1382_5-part2of2.patch. And the comment about optional is not applicable since _mathematica_init_ should never depend on mathematica.

comment:10 Changed 10 years ago by TimothyClemans

William's patch seems to be editing 1382_5.patch. task_1832_2.patch depends on ticket_1382.patch and it doesn't have lines like "return mathematica.mathematica(tuple(self))" which are in 1382_5.patch and show up in red on http://trac.sagemath.org/sage_trac/attachment/ticket/1382/1382_5-part2of2.patch

comment:11 Changed 10 years ago by mabshoff

  • Summary changed from [with patch, with positive review] conversion of sage matrices to mathematica is just completely totally broken to [with patch, with positive review, needs cleaned up patches] conversion of sage matrices to mathematica is just completely totally broken

The patches at this ticket are a mess either way: For task_1832_2.patch I need to ticket_1382.patch. But at that point 1382_5-part2of2.patch doesn't apply cleanly. So, in conclusion: Please post a single patch that has all the changes and that applies cleanly against 2.10.2. To do that merge the fixes back by hand, do not just post a bundle with all patches applied.

Cheers,

Michael

comment:12 Changed 10 years ago by was

I made a typo. Simply apply

  • 1382_5.patch
  • 1382_5-part2of2.patch.

That's it. 2 patches. They should not be flattened.

William

comment:13 Changed 10 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged 1382_5.patch and 1382_5-part2of2.patch in Sage 2.10.3.alpha0. Thanks William for the clarification.

comment:14 Changed 10 years ago by mabshoff

  • Summary changed from [with patch, with positive review, needs cleaned up patches] conversion of sage matrices to mathematica is just completely totally broken to [with patch, with positive review] conversion of sage matrices to mathematica is just completely totally broken
Note: See TracTickets for help on using tickets.