Opened 4 years ago

Closed 19 months ago

#18787 closed defect (wontfix)

symbolic modular integers still broken

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: symbolics Keywords:
Cc: kcrisman, slelievre Merged in:
Authors: Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: u/rws/symbolic_modular_integers_still_broken (Commits) Commit: be9ec4e97557a2ff7ed682ed62c2101d084de498
Dependencies: #21391 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 (38)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 4 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 4 years ago by tmonteil

  • Description modified (diff)

Thanks for isolating the problem.

comment:5 Changed 4 years ago by kcrisman

  • Cc kcrisman added

Thanks very much to both for reporting and simplifying this.

comment:6 follow-up: Changed 4 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 4 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 4 years ago by kcrisman

(Or even just disallow it?)

comment:9 Changed 4 years ago by rws

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

Version 0, edited 4 years ago by rws (next)

comment:10 in reply to: ↑ 6 Changed 4 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 4 years ago by rws (previous) (diff)

comment:11 Changed 4 years ago by rws

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

comment:12 Changed 4 years ago by rws

  • Description modified (diff)

comment:13 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:14 Changed 4 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 4 years ago by rws

  • Branch set to u/rws/bug_with_products_of_symbolic_variables_with_modular_integers

comment:16 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 years ago by rws

  • Milestone changed from sage-6.9 to sage-pending

comment:27 follow-ups: Changed 4 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 4 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.

comment:29 Changed 4 years ago by slelievre

  • Cc slelievre added

comment:30 in reply to: ↑ 27 Changed 3 years ago by rws

Replying to vdelecroix:

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

This both now gives a TypeError in pynac master, equivalently to GF(7)(1) + GF(9)(1) and GF(7)(3) + GF(9)(3).

The ticket should doctest this as soon as pynac is upgraded to 0.6.4

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

comment:31 follow-up: Changed 2 years ago by rws

  • Authors Ralf Stephan deleted
  • Branch u/rws/bug_with_products_of_symbolic_variables_with_modular_integers deleted
  • Commit 6f9e551e2110aa700b29eee6f379ec93890d7ba8 deleted
  • Milestone changed from sage-6.9 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

I believe this is resolved in #21391.

comment:32 in reply to: ↑ 31 Changed 2 years ago by tmonteil

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-7.6
  • Status changed from needs_review to needs_info

Replying to rws:

I believe this is resolved in #21391.

It seems inteed to works better now. But why not adding the tests discussed on the current ticket as you did previously ?

comment:33 Changed 2 years ago by rws

  • Branch set to u/rws/symbolic_modular_integers_still_broken

comment:34 Changed 2 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to be9ec4e97557a2ff7ed682ed62c2101d084de498
  • Dependencies set to #21391
  • Status changed from needs_info to needs_review

Made dependent on #21391 to prevent merge conflicts.


New commits:

714769921391: Disallow mixing of pos.char.ring elements and symbolic variables
dd6ad96Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables
03ccd07Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables
f2933cc21391: introduce ex.has_finite_parent()
333db3cMerge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables
8700e00Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables
c7f8ad221391: to prevent fails give the coordinate functions an explicit characteristic
943060521391: doctest fixes
be9ec4e18787: doctest

comment:35 Changed 20 months ago by mderickx

Do the people on this ticket agree that #24072 solves this ticket as is claimed there?

comment:36 Changed 20 months ago by rws

  • Milestone changed from sage-7.6 to sage-duplicate/invalid/wontfix

Agree.

comment:37 Changed 20 months ago by tscrim

  • Authors Ralf Stephan deleted
  • Reviewers set to Ralf Stephan
  • Status changed from needs_review to positive_review

comment:38 Changed 19 months ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.