Opened 10 years ago

Closed 10 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:

Status badges

Description (last modified by leif)

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)

trac_11401.patch (1.4 KB) - added by nbruin 10 years ago.
straightforward fix
trac_11401v2.patch (4.8 KB) - added by nbruin 10 years ago.
(commit message + "optional -- magma" for examples)
trac_11401v2.2.patch (5.5 KB) - added by nbruin 10 years ago.
compliant commit-message and fix for newlines

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: Changed 10 years ago by jdemeyer

Could this be caused by #9705?

comment:2 in reply to: ↑ 1 Changed 10 years ago by nbruin

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;")
''

Changed 10 years ago by nbruin

straightforward fix

comment:3 Changed 10 years ago by nbruin

  • 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 10 years ago by nbruin

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 10 years ago by nbruin

  • Description modified (diff)

comment:6 Changed 10 years ago by nbruin

  • Component changed from notebook to interfaces

comment:7 follow-up: Changed 10 years ago by mstreng

  • Authors set to Nils Bruin
  • 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

Changed 10 years ago by nbruin

(commit message + "optional -- magma" for examples)

comment:8 in reply to: ↑ 7 Changed 10 years ago by nbruin

  • Owner changed from jason, mpatel, was to (none)

Good catch! fixed.

(amnesic patchbot: apply trac_11401v2.patch)

comment:9 Changed 10 years ago by nbruin

  • Status changed from needs_work to needs_review

comment:10 follow-up: Changed 10 years ago by 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.

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 10 years ago by mstreng

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 10 years ago by nbruin

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)

Changed 10 years ago by nbruin

compliant commit-message and fix for newlines

comment:13 Changed 10 years ago by nbruin

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 10 years ago by mstreng

  • 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 10 years ago by leif

  • 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 10 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.