Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in FractionField conversion #5292

Closed
robertwb opened this issue Feb 17, 2009 · 10 comments
Closed

Error in FractionField conversion #5292

robertwb opened this issue Feb 17, 2009 · 10 comments

Comments

@robertwb
Copy link
Contributor

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

Component: algebra

Issue created by migration from https://trac.sagemath.org/ticket/5292

@robertwb robertwb added this to the sage-3.4.1 milestone Feb 17, 2009
@robertwb
Copy link
Contributor Author

comment:1

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

@robertwb
Copy link
Contributor Author

comment:2

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. = 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.

@robertwb
Copy link
Contributor Author

Attachment: 5292-parsing-fix.patch.gz

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Feb 17, 2009

comment:3

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

Cheers,

Michael

@williamstein
Copy link
Contributor

comment:4

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.

@williamstein
Copy link
Contributor

fixed the previous very broken patch.

@williamstein
Copy link
Contributor

comment:5

Attachment: trac_5292-part2.patch.gz

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

@mwhansen
Copy link
Contributor

comment:6

Looks good to me.

@robertwb
Copy link
Contributor Author

comment:7

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).

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Feb 18, 2009

comment:8

Merged both patches in Sage 3.3.rc2.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Feb 18, 2009
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants