Opened 12 years ago

Closed 12 years ago

#5292 closed defect (fixed)

[with patch, positive review] Error in FractionField conversion

Reported by: robertwb Owned by: tbd
Priority: major Milestone: sage-3.3
Component: algebra Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

On Feb 16, 2009, at 4:01 PM, Jason Bandlow wrote:

sage: R.<x> = QQ[]; S.<q,t> = QQ[]; F = FractionField(S);
sage: x in S   # this is ok
False
sage: x in F   # this is not

ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (857, 0))

ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (862, 0))

---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
...
/home/jason/<string> in <module>()

NameError: name 'x' is not defined

Attachments (2)

5292-parsing-fix.patch (876 bytes) - added by robertwb 12 years ago.
trac_5292-part2.patch (1.2 KB) - added by was 12 years ago.
fixed the previous very broken patch.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 12 years ago by robertwb

This is because eval is being used in lines 585+ of multi_polynomial_libsingular.pyx .

comment:2 Changed 12 years ago by robertwb

  • Milestone changed from sage-3.4.1 to sage-3.3
  • Summary changed from Error in FractionField conversion to [with patch, needs review] Error in FractionField conversion

It's probably bad that it's even using strings here, but this fixes things a bit...

Now, the use of eval() here is bad... for example

sage: sage: R.<x> = QQ[]; S.<q,t> = R[]; F = FractionField?(S); sage: x in S False

is still wrong. There are better ways of going about this but at least it doesn't crash. And I'm more OK with a False negative, but if this is accepted a new ticket should be created to follow up.

Changed 12 years ago by robertwb

comment:3 Changed 12 years ago by mabshoff

With the patch applied to my 3.3.rc2 merge tree all doctests pass.

Cheers,

Michael

comment:4 Changed 12 years ago by was

  • Summary changed from [with patch, needs review] Error in FractionField conversion to [with patch, needs work] Error in FractionField conversion

BAD

15:38 < wstein2> Ouch.
15:38 < wstein2> It has:
15:38 < wstein2> " except SyntaxError, NameError:"
15:38 < wstein2> As a new addition.
15:38 < wstein2> That is a major annoying python gotcha.
15:38 < wstein2> That assigns the exception to NameError.
15:38 < wstein2> It should be
15:38 < wstein2> except (SyntaxError, NameError):
15:38 < wstein2> Ooops!
15:39 < mabs> mk
15:39 < wstein2> I don't know why that would do any good either...
15:40 < wstein2> also, the patch should have a doctest
15:40 < mabs> Hmm, that might be difficult to do.
15:41 < wstein2> in fac the patch does *not* fix the problem.
15:41 < wstein2> You only wrote "all tests pass".
15:41 < wstein2> But that is because there are no new tests.
15:41 < wstein2> That ticket is a mess.
15:41 < mhansen> Patch up for #5298.
15:41 < wstein2> So you wrote: "With the patch applied to my 3.3.rc2 merge tree all doctests pass. "
15:41 < mabs> I did not write that about #5291.
15:41 < wstein2> But there was nothing to test that the problem was fixed.
15:42 < mabs> I wrote that about #5287
15:42 < wstein2> I'm talking about #5292.
15:42 < wstein2> Sorry.
15:42 -!- You're now known as wstein-5292
15:42 < mabs> Yes.

Changed 12 years ago by was

fixed the previous very broken patch.

comment:5 Changed 12 years ago by was

  • Summary changed from [with patch, needs work] Error in FractionField conversion to [with patch, needs review] Error in FractionField conversion

I've attached a patch addressing all my remarks. Somebody review this. Mabshoff -- apply both patches in order.

comment:6 Changed 12 years ago by mhansen

  • Summary changed from [with patch, needs review] Error in FractionField conversion to [with patch, positive review] Error in FractionField conversion

Looks good to me.

comment:7 Changed 12 years ago by robertwb

Ouch, sorry. Yeah, that's one "wart" that I'm glad is being moved: http://www.python.org/dev/peps/pep-3110/

I should have added a test, but as I mentioned I don't think this resolves the real issue here (and since I found exactly where the problem was I wanted to make note of it).

comment:8 Changed 12 years ago by mabshoff

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

Merged both patches in Sage 3.3.rc2.

Cheers,

Michael

Note: See TracTickets for help on using tickets.