Opened 6 years ago

Last modified 3 years ago

#18787 closed defect

symbolic modular integers still broken — at Version 28

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: symbolics Keywords:
Cc: kcrisman, slelievre Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/bug_with_products_of_symbolic_variables_with_modular_integers (Commits) Commit: 6f9e551e2110aa700b29eee6f379ec93890d7ba8
Dependencies: Stopgaps:

Description (last modified by rws)

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'

}}}

Change History (28)

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

  • Cc kcrisman added

Thanks very much to both for reporting and simplifying this.

comment:6 follow-up: 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

Replying to nbruin:

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.
 			               add_dyn(ex_to<numeric>(itin2->coeff));
 			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:

6f9e55118787: doctests

comment:17 follow-up: 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

instead of

sage: x=foo

comment:19 follow-up: 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

Replying to rws:

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: 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: Changed 6 years ago by rws

Replying to jdemeyer:

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

Replying to rws:

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

Replying to jdemeyer:

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: 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

Replying to vdelecroix:

Why is it in sage-pending?

I didn't want patchbot on it.

Note: See TracTickets for help on using tickets.