Opened 6 years ago

Closed 6 years ago

Last modified 6 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 6 years ago.
task_1832_2.patch (1.4 KB) - added by TimothyClemans 6 years ago.
task_1832_3.patch (982 bytes) - added by TimothyClemans 6 years ago.
task_1382_4.patch (1.6 KB) - added by TimothyClemans 6 years ago.
combined_1382.patch (1.6 KB) - added by TimothyClemans 6 years ago.
1382_5.patch (2.1 KB) - added by TimothyClemans 6 years ago.
1382_5-part2of2.patch (2.7 KB) - added by was 6 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 6 years ago by mabshoff

  • Milestone changed from sage-2.10 to sage-2.9.1

Changed 6 years ago by TimothyClemans

comment:2 Changed 6 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 6 years ago by TimothyClemans

Changed 6 years ago by TimothyClemans

Changed 6 years ago by TimothyClemans

comment:3 Changed 6 years ago by TimothyClemans

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

Changed 6 years ago by TimothyClemans

comment:4 Changed 6 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 6 years ago by TimothyClemans

Changed 6 years ago by was

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

comment:5 Changed 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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.