Opened 6 years ago

# symbolic modular integers still broken — at Version 28

Reported by: Owned by: tmonteil major sage-duplicate/invalid/wontfix symbolics kcrisman, slelievre Ralf Stephan N/A u/rws/bug_with_products_of_symbolic_variables_with_modular_integers (Commits) 6f9e551e2110aa700b29eee6f379ec93890d7ba8

```sage: f(x) = Zmod(7)(1) * x**2 + Zmod(9)(1) * x**3
sage: f(1)
2
sage: Zmod(7)(1) + Zmod(9)(1)
```

... TypeError?: unsupported operand parent(s) for '+': 'Ring of integers modulo 7' and 'Ring of integers modulo 9'

}}}

### comment:1 Changed 6 years ago by jdemeyer

• Description modified (diff)

### comment:2 Changed 6 years ago by jdemeyer

• Description modified (diff)

### comment:3 Changed 6 years ago by jdemeyer

• Summary changed from Bug with matrice products over Symbolic Ring with modular integers to Bug with products of symbolic variables with modular integers

### comment:4 Changed 6 years ago by tmonteil

• Description modified (diff)

Thanks for isolating the problem.

### comment:5 Changed 6 years ago by kcrisman

Thanks very much to both for reporting and simplifying this.

### comment:6 follow-up: ↓ 10 Changed 6 years ago by nbruin

A different issue, but it might be related:

```sage: F=sum((i+1)*x^i for i in [0..20])
sage: G=sum(Zmod(7)(i+1)*x^i for i in [0..20])
sage: F*Zmod(7)(1) - G
x^19 + 2*x^18 + 0*x^13 + 0*x^6
sage: G*Zmod(7)(1) - G
0
```

so there seems to be something fishy with symbolic multiplication involving integers and elements of Z/n in general. It's not just a zero-divisor problem.

It's not just powers either:

```sage: V=[SR.var("x%s"%i) for i in [0..20]]
sage: F=sum((i+1)*V[i] for i in [0..20])
sage: G=sum(Zmod(7)(i+1)*V[i] for i in [0..20])
sage: sage: F*Zmod(7)(1)-G
-x0 + 0*x13 + 6*x14 + 0*x20 + 3*x3 + 0*x6
sage: F-G
14*x13 + 21*x20 + 7*x6
```

The last one is correct, since the terms with x6, x20 and x13 are really missing from G. This illustrates why mixing characteristics in SR is always going to be a mess (even if non-zero characteristic works properly otherwise).

### comment:7 Changed 6 years ago by tmonteil

It seems to be a pynac/ginac issue, right ? So what should we do on our side ? Add a stopgap during conversion/coercion Zmod(n) -> SR ?

### comment:8 Changed 6 years ago by kcrisman

(Or even just disallow it?)

### comment:9 Changed 6 years ago by rws

This is working in pynac-0.4.1 (but not 0.3.9.1).

EDIT: I meant the original case.

Last edited 6 years ago by rws (previous) (diff)

### comment:10 in reply to: ↑ 6 Changed 6 years ago by rws

It's not just powers either:

```sage: V=[SR.var("x%s"%i) for i in [0..20]]
sage: F=sum((i+1)*V[i] for i in [0..20])
sage: G=sum(Zmod(7)(i+1)*V[i] for i in [0..20])
sage: sage: F*Zmod(7)(1)-G
-x0 + 0*x13 + 6*x14 + 0*x20 + 3*x3 + 0*x6
sage: F-G
14*x13 + 21*x20 + 7*x6
```

This is different in 0.4.1:

```sage: sage: sage: F*Zmod(7)(1)-G
-x0 + 0*x13 + 0*x20 + 4*x9
sage: F-G
14*x13 + 21*x20 + 7*x6
sage: F
x0 + 2*x1 + 11*x10 + 12*x11 + 13*x12 + 14*x13 + 15*x14 + 16*x15 + 17*x16 + 18*x17 + 19*x18 + 20*x19 + 3*x2 + 21*x20 + 4*x3 + 5*x4 + 6*x5 + 7*x6 + 8*x7 + 9*x8 + 10*x9
sage: G
x0 + 2*x1 + 4*x10 + 5*x11 + 6*x12 + x14 + 2*x15 + 3*x16 + 4*x17 + 5*x18 + 6*x19 + 3*x2 + 4*x3 + 5*x4 + 6*x5 + x7 + 2*x8 + 3*x9
```

EDIT: Added `F`,`G`.

Last edited 6 years ago by rws (previous) (diff)

### comment:11 Changed 6 years ago by rws

Correction: this is not pynac-0.4.1 but pynac master. Narrowing down the reponsible change...

### comment:12 Changed 6 years ago by rws

• Description modified (diff)

### comment:13 Changed 6 years ago by jdemeyer

• Description modified (diff)

### comment:14 Changed 6 years ago by rws

• Dependencies set to pynac-0.3.9.2
• Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

The following patch works for these two examples, just to point at the direction further completion is needed:

```diff --git a/ginac/expairseq.cpp b/ginac/expairseq.cpp
index 2abb7b7..acef09b 100644
--- a/ginac/expairseq.cpp
+++ b/ginac/expairseq.cpp
@@ -1207,16 +1207,17 @@ void expairseq::combine_same_terms_sorted_seq()
// must_copy will be set to true the first time some combination is
// possible from then on the sequence has changed and must be compacted
bool must_copy = false;
+        numeric itin1_c = ex_to<numeric>(itin1->coeff);
while (itin2!=last) {
if (itin1->rest.compare(itin2->rest)==0 &&
unlikely(!is_a<infinity>(itin1->rest))) {
-			itin1->coeff = ex_to<numeric>(itin1->coeff).
+			itin1->coeff = itin1_c.
if (expair_needs_further_processing(itin1))
needs_further_processing = true;
must_copy = true;
} else {
-			if (!ex_to<numeric>(itin1->coeff).is_zero()) {
+			if (not itin1_c.is_zero() or itin1_c.is_parent_pos_char()) {
if (must_copy)
*itout = *itin1;
++itout;
@@ -1225,7 +1226,7 @@ void expairseq::combine_same_terms_sorted_seq()
}
++itin2;
}
-	if (!ex_to<numeric>(itin1->coeff).is_zero()) {
+	if (not itin1_c.is_zero() or itin1_c.is_parent_pos_char()) {
if (must_copy)
*itout = *itin1;
++itout;
```

The relevant ticket would be https://github.com/pynac/pynac/issues/77.

### comment:15 Changed 6 years ago by rws

• Branch set to u/rws/bug_with_products_of_symbolic_variables_with_modular_integers

### comment:16 Changed 6 years ago by rws

• Authors set to Ralf Stephan
• Commit set to 6f9e551e2110aa700b29eee6f379ec93890d7ba8
• Dependencies pynac-0.3.9.2 deleted
• Report Upstream changed from Reported upstream. Developers acknowledge bug. to N/A
• Status changed from new to needs_review
• Summary changed from Bug with products of symbolic variables with modular integers to doctest symbolic modular integers fix

New commits:

 ​6f9e551 `18787: doctests`

### comment:17 follow-up: ↓ 24 Changed 6 years ago by jdemeyer

• Status changed from needs_review to needs_work

With Sage-6.8.rc0:

```**********************************************************************
File "src/sage/symbolic/expression.pyx", line 2821, in sage.symbolic.expression.Expression._add_
Failed example:
(A + 3*B)*Zmod(9)(6)
Expected:
6*A
Got:
0* + 6*A
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 2823, in sage.symbolic.expression.Expression._add_
Failed example:
(3*A + 3*B)*Zmod(9)(6)
Expected:
0
Got:
0*
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 2825, in sage.symbolic.expression.Expression._add_
Failed example:
(3*A + 3*B)*Zmod(9)(6)*A
Exception raised:
Traceback (most recent call last):
File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.symbolic.expression.Expression._add_[24]>", line 1, in <module>
(Integer(3)*A + Integer(3)*B)*Zmod(Integer(9))(Integer(6))*A
File "sage/structure/element.pyx", line 1849, in sage.structure.element.RingElement.__mul__ (build/cythonized/sage/structure/element.c:16581)
return (<RingElement>left)._mul_(<RingElement>right)
File "sage/symbolic/expression.pyx", line 3148, in sage.symbolic.expression.Expression._mul_ (build/cythonized/sage/symbolic/expression.cpp:19837)
x = gmul(left._gobj, _right._gobj)
OverflowError: numeric::div(): division by zero
**********************************************************************
```

### comment:18 Changed 6 years ago by jdemeyer

Also a detail: it's better style to use

```sage: x = foo
```

```sage: x=foo
```

### comment:19 follow-up: ↓ 20 Changed 6 years ago by rws

Seems the backport from Pynac-0.4.2 missed something. When will C++11 be mandatory in Sage?

### comment:20 in reply to: ↑ 19 Changed 6 years ago by jdemeyer

When will C++11 be mandatory in Sage?

I think I already mentioned this: I recommend you write to sage-devel with this request.

### comment:21 follow-up: ↓ 22 Changed 6 years ago by jdemeyer

Let me add that the hard part will be to define the precise subset of C++11 that you need.

It's easy to require that the C++ compiler supports the `-std=c++0x` or `-std=c++11` option, but I don't know if that is sufficient.

### comment:22 in reply to: ↑ 21 ; follow-up: ↓ 23 Changed 6 years ago by rws

Let me add that the hard part will be to define the precise subset of C++11 that you need.

Why? We have already committed to gcc-4.6 in our conversation. For the other system without gcc there will have to be an Xcode version that has equivalent functionality. These two versions will be made the minimum. No other definition necessary.

### comment:23 in reply to: ↑ 22 Changed 6 years ago by jdemeyer

We have already committed to gcc-4.6 in our conversation

Really? I understood that you were not sure if GCC 4.6 was in fact sufficient. But if you are, that's fine for me.

Note that, for various reasons, we actually do not support GCC 4.6 in Sage, which would make GCC 4.7 the minimum.

If you're willing to go ahead with this, you should:

1. write to `sage-devel` explaining why you want C++11 support and that this means requiring GCC 4.7.
2. create a ticket for requiring C++11 and GCC 4.7 in Sage.
3. create a ticket for upgrading Pynac to the version and make it depend on the ticket in 2.

### comment:24 in reply to: ↑ 17 Changed 5 years ago by rws

• Dependencies set to pynac-0.3.9.3/-0.4.2
• Milestone changed from sage-6.8 to sage-6.9

With Sage-6.8.rc0:

```Got:
0* + 6*A
Got:
0*
```

Found it. The backport was indeed incomplete. Fix in pynac-0.3.9.3.

### comment:25 Changed 5 years ago by rws

• Dependencies changed from pynac-0.3.9.3/-0.4.2 to #18980
• Status changed from needs_work to needs_review

### comment:26 Changed 5 years ago by rws

• Milestone changed from sage-6.9 to sage-pending

### comment:27 follow-up: ↓ 28 Changed 5 years ago by vdelecroix

• Status changed from needs_review to needs_info

Why is it in sage-pending?

What about the following (inspired from comment:6

```sage: f(x) = Zmod(7)(3) * x**2 + Zmod(9)(3) * x**3
sage: f(1)
** BOOM **
```

while

```sage: f(x) = Zmod(7)(1) * x**2 + Zmod(9)(1) * x**3
sage: f(1)
2
```

Vincent

### comment:28 in reply to: ↑ 27 Changed 5 years ago by rws

• Dependencies #18980 deleted
• Description modified (diff)
• Milestone changed from sage-pending to sage-6.9
• Status changed from needs_info to needs_work
• Summary changed from doctest symbolic modular integers fix to symbolic modular integers still broken