#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)
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
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
This will not work, since it does not appear to be recursive. For example:
Also, please post a unified patch making it easy to see just the total changes.