Opened 4 years ago
Closed 18 months ago
#18787 closed defect (wontfix)
symbolic modular integers still broken
Reported by:  tmonteil  Owned by:  

Priority:  major  Milestone:  sageduplicate/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 )
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
 Description modified (diff)
comment:2 Changed 4 years ago by
 Description modified (diff)
comment:3 Changed 4 years ago by
 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
 Description modified (diff)
comment:5 Changed 4 years ago by
 Cc kcrisman added
Thanks very much to both for reporting and simplifying this.
comment:6 followup: ↓ 10 Changed 4 years ago by
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 zerodivisor 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: FG 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 nonzero characteristic works properly otherwise).
comment:7 Changed 4 years ago by
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
(Or even just disallow it?)
comment:9 Changed 4 years ago by
This is working in pynac0.4.1 (but not 0.3.9.1).
EDIT: I meant the original case.
comment:10 in reply to: ↑ 6 Changed 4 years ago by
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: FG 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: FG 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
.
comment:11 Changed 4 years ago by
Correction: this is not pynac0.4.1 but pynac master. Narrowing down the reponsible change...
comment:12 Changed 4 years ago by
 Description modified (diff)
comment:13 Changed 4 years ago by
 Description modified (diff)
comment:14 Changed 4 years ago by
 Dependencies set to pynac0.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
 Branch set to u/rws/bug_with_products_of_symbolic_variables_with_modular_integers
comment:16 Changed 4 years ago by
 Commit set to 6f9e551e2110aa700b29eee6f379ec93890d7ba8
 Dependencies pynac0.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 followup: ↓ 24 Changed 4 years ago by
 Status changed from needs_review to needs_work
With Sage6.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/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/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
Also a detail: it's better style to use
sage: x = foo
instead of
sage: x=foo
comment:19 followup: ↓ 20 Changed 4 years ago by
Seems the backport from Pynac0.4.2 missed something. When will C++11 be mandatory in Sage?
comment:20 in reply to: ↑ 19 Changed 4 years ago by
Replying to rws:
When will C++11 be mandatory in Sage?
I think I already mentioned this: I recommend you write to sagedevel with this request.
comment:21 followup: ↓ 22 Changed 4 years ago by
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 ; followup: ↓ 23 Changed 4 years ago by
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 gcc4.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
Replying to rws:
We have already committed to gcc4.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:
 write to
sagedevel
explaining why you want C++11 support and that this means requiring GCC 4.7.  create a ticket for requiring C++11 and GCC 4.7 in Sage.
 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
 Dependencies set to pynac0.3.9.3/0.4.2
 Milestone changed from sage6.8 to sage6.9
Replying to jdemeyer:
With Sage6.8.rc0:
Got: 0* + 6*A Got: 0*
Found it. The backport was indeed incomplete. Fix in pynac0.3.9.3.
comment:25 Changed 4 years ago by
 Dependencies changed from pynac0.3.9.3/0.4.2 to #18980
 Status changed from needs_work to needs_review
comment:26 Changed 4 years ago by
 Milestone changed from sage6.9 to sagepending
comment:27 followups: ↓ 28 ↓ 30 Changed 4 years ago by
 Status changed from needs_review to needs_info
Why is it in sagepending?
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
 Dependencies #18980 deleted
 Description modified (diff)
 Milestone changed from sagepending to sage6.9
 Status changed from needs_info to needs_work
 Summary changed from doctest symbolic modular integers fix to symbolic modular integers still broken
comment:29 Changed 4 years ago by
 Cc slelievre added
comment:30 in reply to: ↑ 27 Changed 3 years ago by
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
comment:31 followup: ↓ 32 Changed 2 years ago by
 Branch u/rws/bug_with_products_of_symbolic_variables_with_modular_integers deleted
 Commit 6f9e551e2110aa700b29eee6f379ec93890d7ba8 deleted
 Milestone changed from sage6.9 to sageduplicate/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
 Milestone changed from sageduplicate/invalid/wontfix to sage7.6
 Status changed from needs_review to needs_info
comment:33 Changed 2 years ago by
 Branch set to u/rws/symbolic_modular_integers_still_broken
comment:34 Changed 2 years ago by
 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:
7147699  21391: Disallow mixing of pos.char.ring elements and symbolic variables

dd6ad96  Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables

03ccd07  Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables

f2933cc  21391: introduce ex.has_finite_parent()

333db3c  Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables

8700e00  Merge branch 'develop' into t/21391/disallow_mixing_of_pos_char_ring_elements_and_symbolic_variables

c7f8ad2  21391: to prevent fails give the coordinate functions an explicit characteristic

9430605  21391: doctest fixes

be9ec4e  18787: doctest

comment:35 Changed 19 months ago by
Do the people on this ticket agree that #24072 solves this ticket as is claimed there?
comment:36 Changed 19 months ago by
 Milestone changed from sage7.6 to sageduplicate/invalid/wontfix
Agree.
comment:37 Changed 19 months ago by
 Reviewers set to Ralf Stephan
 Status changed from needs_review to positive_review
comment:38 Changed 18 months ago by
 Resolution set to wontfix
 Status changed from positive_review to closed
Thanks for isolating the problem.