Opened 10 years ago
Closed 8 years ago
#13609 closed defect (fixed)
symbolic arithmetic errors
Reported by:  llpamies  Owned by:  burcin 

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 changed from PLEASE CHANGE to symbolics
 Owner changed from tbd to burcin
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
 Dependencies set to #13729
 Keywords pynac segfault added
 Status changed from new to 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 changed from #13729 to #13729, #13736
 Description modified (diff)
 Reviewers set to 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:7 Changed 9 years ago by
Fuzz 2 on top of sage5.10.beta2, so needs to be rebased.
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 changed from sage5.11 to sage5.12
comment:11 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:12 Changed 8 years ago by
 Branch set to u/mmezzarobba/ticket/13609
 Commit set to 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 8 years ago by
I don't really remember, but if you feel happy with the changes let's merge this.
comment:14 Changed 8 years ago by
 Status changed from needs_review to positive_review
comment:15 followup: ↓ 16 Changed 8 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 in reply to: ↑ 15 Changed 8 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 8 years ago by
 Branch changed from u/mmezzarobba/ticket/13609 to e0d15a9bf67e292d23ff306e2a8e632a280debea
 Resolution set to fixed
 Status changed from positive_review to 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: