Opened 11 years ago
Closed 11 years ago
#11401 closed defect (fixed)
magma mode in 4.7 notebook broken
Reported by: | nbruin | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | interfaces | Keywords: | magma notebook interface |
Cc: | Merged in: | sage-4.7.2.alpha3 | |
Authors: | Nils Bruin | Reviewers: | Marco Streng |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
See also this sage-devel thread
It seems that magma mode in the 4.7 notebook is broken. I observed the following session after verifying John Cremona's report about syncing problems. I suspect sync is lost due to the multiple commands. Note that none of the print commands seem to get executed (no output is observed) and that for "print 7;" the line "print 6;" gets echoed (this has probably very little to do with the "print 7;" command itself)
a:=1; b:=2; c:=3; /// b:=2; c:=3;
print 4; ///
d:=4; print 5; ///
print 6; ///
print 7; /// print 6;
Apply only trac_11401v2.2.patch to the Sage library.
Attachments (3)
Change History (19)
comment:1 follow-up: ↓ 2 Changed 11 years ago by
comment:2 in reply to: ↑ 1 Changed 11 years ago by
Could this be caused by #9705?
I guess it could be caused by its fix ... The cells there execute without problem for me in 4.7 and those cells do contain multiple commands that do not produce output. It seems that the input (in one cell)
a:=1; b:=2;
reliably knocks the interface out of sync. So probably if someone who know how these interfaces work, looks at the strings that are passed back and forth, it will become apparent quite quickly.
I don't think the problem is confined to the notebook either. I observed:
sage: magma.execute(""" ....: a:=1; ....: b:=2; ....: """) 'b:=2;' sage: magma.execute("print 3;") ''
comment:3 Changed 11 years ago by
- Status changed from new to needs_review
Perhaps a little too naive of a fix and perhaps focussing a little too much on the symptom rather than the cause, but if newlines are causing problems, why not just replace them with spaces? In magma syntax, both newline and space are whitespace characters and those are all treated the same. Attached fix does pass
sage -t --optional --long magma.py
which I suppose contains all magma interfacing doctests?
comment:4 Changed 11 years ago by
I think I now have a proper fix for the problem. The problem is indeed caused by the fix of #9705. The matter is the following (and this will be true for a lot of interfaces):
- Magma essentially responds to each newline with a new prompt. So, when you are feeding input to magma via its stdin, the most straightforward way is to do this line by line and eat the prompts as you go along. This happened before the fix of #9705.
- The decision to transfer via file or via stdin was made on a line-by-line basis. So, newlines inside expressions could lead to input being split over several files and/or a bit of stdin. This caused #9705. The fix introduced there solves the problem by introducing split_lines. With split_lines=False, one can end up with multi-line input in expect._eval_line. When _eval_line decides to communicate those lines via stdin then there is a mismatch between the number of prompts generated and the number expected.
The solution in trac_11401v2.patch
is to move the decision to use files to "eval". This makes it possible to either transmit all code via a temp file or to send the input line-by-line to stdin (and those lines will never trigger file use). I think this is also more efficient because it favours transfer in bigger blocks (before the #9705 fix, input consisting of many short lines would still be communicated via stdin. Now it will be communicated via one tmp file)
This means that the split_lines
option can now take 3 values. The option "use one file OR split lines" is the right behaviour for magma and probably for most other interfaces too.
I have also added a doctest testing that #11401 is fixed. I also reintroduced the newlines in the test for #9705. Those are actually essential for testing (the version without newlines always worked as expected).
comment:5 Changed 11 years ago by
- Description modified (diff)
comment:6 Changed 11 years ago by
- Component changed from notebook to interfaces
comment:7 follow-up: ↓ 8 Changed 11 years ago by
- Reviewers set to Marco Streng
- Status changed from needs_review to needs_work
I don't have a machine without Magma to test this on, but I'm pretty sure that the examples "sage: magma.eval("a:=3;\nb:=5;")
" and "magma.eval("[a,b];")
" need "# optional - magma
".
ps. for the patchbot which didn't seem to have gotten the message: apply trac_11401v2.patch
comment:8 in reply to: ↑ 7 Changed 11 years ago by
- Owner changed from jason, mpatel, was to (none)
Good catch! fixed.
(amnesic patchbot: apply trac_11401v2.patch)
comment:9 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:10 follow-up: ↓ 12 Changed 11 years ago by
Why are you changing the example for "Verify that trac 9705 is fixed"? I don't see why one is better than the other.
To check for myself that your new version of that test really verifies that trac 9705 is fixed, I would have to install a pre-#9705 version of Sage and see that that test fails. That's a bit more work than I would like to do.
Patchbot, would you apply trac_11401v2.patch please?
comment:11 Changed 11 years ago by
All tests pass. Patch looks good. I haven't tried the notebook, but the patch does what it should do to multi-line magma.eval.
That leaves just the one question about the changed #9705 doctest.
comment:12 in reply to: ↑ 10 Changed 11 years ago by
Replying to mstreng:
Why are you changing the example for "Verify that trac 9705 is fixed"? I don't see why one is better than the other.
Because the example without newlines runs OK both with and without the fix of #9705, so the test is not verifying any change in behaviour. The problem described in #9705 arose when one magma statement was spread over multiple lines and one line was passed over stdin and another line via a "load" command. Without newlines, such splits wouldn't occur anyway.
If people can't find a fix for the "\n" in the tests, I guess we could write it in the following way:
sage: import commands sage: newline=commands.getoutput("echo; echo") sage: command=( "_<x>:=PolynomialRing(Rationals());"+newline+ "repeat"+newline+ " g:=3*b*x^4+18*c*x^3-6*b^2*x^2-6*b*c*x-b^3-9*c^2 where b:=Random([-10..10]) where c:=Random([-10..10]);"+newline+ "until Roots(g) ne [];") sage: magma.eval(command)
comment:13 Changed 11 years ago by
OK, latest version should provide proper formatting of documentation _and_ have newlines in doctests. The doc formatting is surprisingly fragile in the face of multi-line strings, so manually adding the newline characters seems to be the best idea. Plus, second lines of multiline strings get preparsed by the doctester! That wreaks havoc with magma code (Integer(3) etc.)
Dear patchbot, would you be so kind to just apply trac_11401v2.2.patch please?
comment:14 Changed 11 years ago by
- Status changed from needs_review to positive_review
That new version looks good, and does the trick.
I'm glad you didn't really do newline=commands.getoutput("echo; echo")
comment:15 Changed 11 years ago by
- Description modified (diff)
Please use proper URLs, or in this case, trac wiki mark-up ("[attachment:here_comes_the_filename]
") to reference attached patches in the description.
comment:16 Changed 11 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Could this be caused by #9705?