Opened 10 years ago
Closed 9 years ago
#13609 closed defect (fixed)
symbolic arithmetic errors
Reported by:  Lluis PamiesJuarez  Owned by:  Burcin Erocal 

Priority:  major  Milestone:  sage6.2 
Component:  symbolics  Keywords:  pynac segfault 
Cc:  Merged in:  
Authors:  Burcin Erocal  Reviewers:  JeanPierre Flori 
Report Upstream:  N/A  Work issues:  
Branch:  e0d15a9 (Commits, GitHub, GitLab)  Commit:  e0d15a9bf67e292d23ff306e2a8e632a280debea 
Dependencies:  #13729, #13736  Stopgaps: 
Description (last modified by )
Consider the following code:
ff.<z> = GF(2**8, 'z') poly.<c1,c2,c3> = PolynomialRing(ff, 3, 'c') r1,r2 = var('r1,r2') expression = (c1*r2  c2*r1)/c3 print expression.substitute(r1=z, r2=z)
This produces a TypeError?: unsupported operand parent(s) for '*': 'Finite Field in z of size 2pow8' and 'Rational Field'. I know that 'expression' is not an element of the ring 'poly', but using a PolynomialRing? is the only way I found to achieve symbolic arithmetic on finite fields.
However, the interesting story is that if I replace the expression by
expression = (r2  c2*r1)/c3
it work perfectly well, but if instead the expression is
expression = (c1 + r2  c2*r1)/c3
then I get a segmentation fault.
To make things a little bit more interesting I can rename r1 and r2 to a and b:
ff.<z> = GF(2**8, 'z') poly.<c1,c2,c3> = PolynomialRing(ff, 3, 'c') a,b = var('a,b') expression = (c1*b  c2*a)/c3 print expression.substitute(a=z, b=z)
Then it works fine, but produces a segmentation fault for,
expression = (c1 + b  c2*a)/c3
so you can think that it might be a problem with the use of the names r1 and r2. But this is not the case, if I rename the PolynomialRing? variables instead, from c's to x's:
ff.<z> = GF(2**8, 'z') poly.<x1,x2,x3> = PolynomialRing(ff, 3, 'x') r1,r2 = var('r1,r2') expression = (x1*r2  x2*r1)/x3 print expression.substitute(r1=z, r2=z)
Then it works again for the first two expressions but produces a segmentation fault for the third too.
Any idea of what is going wrong here?
Apply trac_13609rebase.patch
Attachments (2)
Change History (19)
comment:1 Changed 10 years ago by
Component:  PLEASE CHANGE → symbolics 

Owner:  changed from tbd to Burcin Erocal 
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
I opened a ticket on the Pynac issue tracker. I'll come up with a patch soon.
comment:4 Changed 10 years ago by
Authors:  → Burcin Erocal 

Dependencies:  → #13729 
Keywords:  pynac segfault added 
Status:  new → needs_review 
Pynac 0.2.6 contains a fix for this. Updating the spkg is #13729.
comment:5 Changed 10 years ago by
Would it be possible to give a little more details in the doctests introducing text? like explaining that depending on the name/ordering of the variables, the bug was/ was not triggered, whence the different tests made.
And putting expression or ex everywhere rather than changing the name in the middle?
With this I'll happily give positive review.
comment:6 Changed 10 years ago by
Dependencies:  #13729 → #13729, #13736 

Description:  modified (diff) 
Reviewers:  → JeanPierre Flori 
I uploaded a new patch with some text trac_13609gf2_content.take2.patch. To demonstrate the content of the expressions involved, I wrapped GiNaC's content()
method in #13736. This ticket now depends on the patch there.
Note that multiplying the numerator I give in the doctests by the content gives
sage: num.content(c1)*num c1 + z*c2 + z
which changes the original leading coefficient 1
by coercing it to GF(2^8)
.
This does not happen during normalization, since numeric::div_dyn()
is used
directly to modify the coefficients. There is a shortcut in that function to
do nothing if we are dividing by 1
. Perhaps I should back out the current
fix in Pynac and change numeric::div_dyn()
to disable the shortcut if the
characteristic is not 0.
Comments?
comment:8 Changed 9 years ago by
Description:  modified (diff) 

comment:9 Changed 9 years ago by
Perhaps I should back out the current fix in Pynac and change numeric::div_dyn() to disable the shortcut if the characteristic is not 0.
I think it is reasonable to have different behavior outside char 0.
comment:10 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:11 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:12 Changed 9 years ago by
Branch:  → u/mmezzarobba/ticket/13609 

Commit:  → e0d15a9bf67e292d23ff306e2a8e632a280debea 
The fix has been in Sage's version of Pynac for one year. The patch attached to this ticket (which just adds new tests) applies cleanly to sage6.2.beta4, and the new tests pass. Is there any reason this ticket still needs review?
New commits:
e0d15a9  Trac 13609: fix symbolic expression auto evaluation when content is in GF(2^k)

comment:13 Changed 9 years ago by
I don't really remember, but if you feel happy with the changes let's merge this.
comment:14 Changed 9 years ago by
Status:  needs_review → positive_review 

comment:15 followup: 16 Changed 9 years ago by
If there is no mathematical problem it's okay; I think that Burcin's point was that sometimes plus and minus one can replace each other with this code sometimes in characteristic two. Which is ... true, they can. It just doesn't "look" the same. Or at least I think that was the point of his comment and doctest along those lines?
comment:16 Changed 9 years ago by
Replying to kcrisman:
If there is no mathematical problem it's okay; I think that Burcin's point was that sometimes plus and minus one can replace each other with this code sometimes in characteristic two. Which is ... true, they can. It just doesn't "look" the same. Or at least I think that was the point of his comment and doctest along those lines?
In any case, the patch in this ticket only documents the currently implemented behaviour. I'd say any changes to address the behaviour noted in Burcin's comment belong in another ticket.
comment:17 Changed 9 years ago by
Branch:  u/mmezzarobba/ticket/13609 → e0d15a9bf67e292d23ff306e2a8e632a280debea 

Resolution:  → fixed 
Status:  positive_review → closed 
I can confirm the segfaults. The following three examples lead to a crash:
The fact that this depends on variable names made me think of ordering issues like #9880, but the examples above still lead to a segfault with patches from that ticket. I am now suspecting a coercion problem.
BTW, the backtrace looks like this: