Opened 4 years ago
Closed 3 years ago
#23719 closed enhancement (fixed)
Move matrices to new coercion model
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  coercion  Keywords:  days94 
Cc:  tscrim  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  b03b8bf (Commits, GitHub, GitLab)  Commit:  b03b8bf02a33a4cd5da4fa8e15d0e528f4efacdf 
Dependencies:  #25504, #25505, #25542, #25554, #25555  Stopgaps: 
Description (last modified by )
MatrixSpace
do not play well with the category framework. One step forward is to rename the attribute _matrix_class
to Element
so that it will have element_class
pointing to the element class generated via Parent.__init__
.
Related tickets: #19669
Change History (51)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
 Component changed from linear algebra to coercion
 Dependencies changed from #23706 to #24742
 Summary changed from MatrixSpace: replace _matrix_class with element_class to Move matrices to new coercion model
comment:3 Changed 3 years ago by
 Description modified (diff)
 Milestone changed from sage8.1 to sage8.2
comment:4 Changed 3 years ago by
comment:5 Changed 3 years ago by
 Description modified (diff)
comment:6 Changed 3 years ago by
 Dependencies #24742 deleted
 Milestone changed from sage8.2 to sage8.3
comment:8 Changed 3 years ago by
Vincent, since you created this ticket: are there any concrete issues that would be fixed by this?
comment:9 followup: ↓ 10 Changed 3 years ago by
My aim was to allow matrices on semirings (ie structure having +
and *
but not necessarily 
and /
). We are far away from there though. So only a wishlist.
comment:10 in reply to: ↑ 9 Changed 3 years ago by
Replying to vdelecroix:
My aim was to allow matrices on semirings
What do you mean with "allow"? And how would that be related to changing element_class
?
comment:11 Changed 3 years ago by
 Branch set to u/jdemeyer/move_matrices_to_new_coercion_model
comment:12 Changed 3 years ago by
 Commit set to e48d153eeee6cc6180abc61a96cae6e618784733
comment:13 Changed 3 years ago by
 Dependencies set to #25504
comment:14 Changed 3 years ago by
 Dependencies changed from #25504 to #25504, #25505
comment:15 Changed 3 years ago by
 Commit changed from e48d153eeee6cc6180abc61a96cae6e618784733 to c646dc23a2a0306d03f358b4f1a6f65620805185
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
3c4a06d  Enable _mul_long for matrices

1eaed37  More stuff in the meataxe interface, and a meataxe helper function

8a06c0f  Fix docstring formatting

13da208  Clean up creating Matrix_gfpn_dense matrices

cb52ee3  Mark one doctest optional

c8fa7bb  Clean up __cinit__ methods of matrices

779dffb  Implement _an_element_ for matrix spaces

6b8517f  Merge commit '779dffbbb514290400d2b164b80ddb1a6c8ba04e' into t/23719/move_matrices_to_new_coercion_model

55568c8  Misc matrix fixes

c646dc2  Move matrices to new coercion model

comment:16 Changed 3 years ago by
 Dependencies changed from #25504, #25505 to #25504, #25505, #25542
comment:17 Changed 3 years ago by
 Status changed from new to needs_review
comment:19 Changed 3 years ago by
 Commit changed from c646dc23a2a0306d03f358b4f1a6f65620805185 to 8769af1d2248e8193ac95007adcb038df46a3ef0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b3901d7  Misc matrix fixes

c8ce255  Merge commit 'c8fa7bb27f4724a7263d76bec437ebcf7f87dec3'; commit 'b3901d7dea1ac4bcf7b8c4e2d7adf709b92536cd' into t/23719/move_matrices_to_new_coercion_model

8769af1  Move matrices to new coercion model

comment:20 Changed 3 years ago by
 Dependencies changed from #25504, #25505, #25542 to #25504, #25505, #25542, #25554
comment:21 Changed 3 years ago by
 Dependencies changed from #25504, #25505, #25542, #25554 to #25504, #25505, #25542, #25554, #25555
comment:22 Changed 3 years ago by
 Commit changed from 8769af1d2248e8193ac95007adcb038df46a3ef0 to b6fd971e30aa6a7504e3513905dc82ee249ae74f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
154dbd5  Replace _coerce_ checks by has_coerce_map_from

15f31c0  Support newstyle Parents in RingMap_lift

c821441  Merge commit 'c8fa7bb27f4724a7263d76bec437ebcf7f87dec3'; commit 'b3901d7dea1ac4bcf7b8c4e2d7adf709b92536cd'; commit '154dbd5f68f71bd64855093da869debdbdda1925'; commit '15f31c09ce3973845f2bff779f43bf924be83f14' into t/23719/move_matrices_to_new_coercion_model

b6fd971  Move matrices to new coercion model

comment:23 Changed 3 years ago by
 Commit changed from b6fd971e30aa6a7504e3513905dc82ee249ae74f to 4fa13cdd8c3ed1551eec834362fe467a229ff339
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e1bb9b6  Replace _coerce_ checks by has_coerce_map_from

0b37559  Merge commit '15f31c09ce3973845f2bff779f43bf924be83f14'; commit 'c8fa7bb27f4724a7263d76bec437ebcf7f87dec3'; commit 'b3901d7dea1ac4bcf7b8c4e2d7adf709b92536cd'; commit 'e1bb9b67354593d944aa38864b55112fbdedfec5' into t/23719/move_matrices_to_new_coercion_model

4fa13cd  Move matrices to new coercion model

comment:24 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:25 Changed 3 years ago by
All tests passed!
comment:26 followup: ↓ 27 Changed 3 years ago by
What is the cause of this change
sage: A[4055]*v  (k002*k003) + (k001*k003)
comment:27 in reply to: ↑ 26 Changed 3 years ago by
Replying to vdelecroix:
What is the cause of this change
sage: A[4055]*v  (k002*k003) + (k001*k003)
I guess the answer is random and the random number state is different at that point because of this ticket.
comment:28 Changed 3 years ago by
I just realized that this hasn't been reviewed yet...
Ping?
comment:29 Changed 3 years ago by
 Status changed from needs_review to needs_work
Oops, merge failure...
comment:30 Changed 3 years ago by
 Commit changed from 4fa13cdd8c3ed1551eec834362fe467a229ff339 to 4e76683904bd815de104fac43251d35fddfd5193
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4e76683  Move matrices to new coercion model

comment:31 Changed 3 years ago by
 Commit changed from 4e76683904bd815de104fac43251d35fddfd5193 to 5c6e09ebe03308a2b6141957046eb5e083e7c87b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5c6e09e  Move matrices to new coercion model

comment:32 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:33 Changed 3 years ago by
 Commit changed from 5c6e09ebe03308a2b6141957046eb5e083e7c87b to 0ae7a1c0187769dfadee78e1ad6d8190eceea909
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0ae7a1c  Move matrices to new coercion model

comment:34 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
LGTM assuming a patchbot comes back clean. Vincent, any other comments?
comment:35 Changed 3 years ago by
 Commit changed from 0ae7a1c0187769dfadee78e1ad6d8190eceea909 to 9715f55d97776b31e64d96e356291f12971b1ac0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9715f55  Move matrices to new coercion model

comment:36 Changed 3 years ago by
 Keywords days94 added
comment:37 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:38 Changed 3 years ago by
 Status changed from positive_review to needs_work
I'm getting this error when running the testsuite on 32bit but can't reproduced it when running the test in isolation. But its definitely from this ticket:
File "src/sage/schemes/elliptic_curves/padics.py", line 315, in sage.schemes.elliptic_curves.padics.padic_regulator Failed example: E.padic_regulator(int(5)) Exception raised: Traceback (most recent call last): File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 573, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 983, in compile_and_execute exec(compiled, globs) File "<doctest sage.schemes.elliptic_curves.padics.padic_regulator[14]>", line 1, in <module> E.padic_regulator(int(Integer(5))) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 339, in padic_regulator height = self.padic_height(p, prec, check_hypotheses=False) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 756, in padic_height sigma = self.padic_sigma(p, adjusted_prec, check_hypotheses=False) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 1095, in padic_sigma E2 = self.padic_E2(p, N2, check_hypotheses=False) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 1503, in padic_E2 frob_p = self.matrix_of_frobenius(p, prec, check, check_hypotheses, algorithm).change_ring(Integers(p**prec)) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 1653, in matrix_of_frobenius Q, p, adjusted_prec, trace) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 1658, in matrix_of_frobenius F1_reduced = reduce_all(Q, p, F1_coeffs, offset) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 1024, in reduce_all exact_form = reduce_negative(Q, p, coeffs, offset, exact_form) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 800, in reduce_negative a[1] = base_ring(lift(a[1]) / (j+1)) File "sage/structure/parent.pyx", line 920, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9734) return mor._call_(x) File "sage/structure/coerce_maps.pyx", line 145, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4555) raise File "sage/structure/coerce_maps.pyx", line 140, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4423) return C._element_constructor(x) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/rings/finite_rings/integer_mod_ring.py", line 1167, in _element_constructor_ return integer_mod.IntegerMod(self, x) File "sage/rings/finite_rings/integer_mod.pyx", line 197, in sage.rings.finite_rings.integer_mod.IntegerMod (build/cythonized/sage/rings/finite_rings/integer_mod.c:4558) return t(parent, value) File "sage/rings/finite_rings/integer_mod.pyx", line 361, in sage.rings.finite_rings.integer_mod.IntegerMod_abstract.__init__ (build/cythonized/sage/rings/finite_rings/integer_mod.c:5739) z = value % self.__modulus.sageInteger File "sage/rings/rational.pyx", line 2869, in sage.rings.rational.Rational.__mod__ (build/cythonized/sage/rings/rational.c:25810) d = d.inverse_mod(other) File "sage/rings/integer.pyx", line 6574, in sage.rings.integer.Integer.inverse_mod (build/cythonized/sage/rings/integer.c:41104) raise ZeroDivisionError("Inverse does not exist.") ZeroDivisionError: Inverse does not exist.
comment:39 Changed 3 years ago by
 Commit changed from 9715f55d97776b31e64d96e356291f12971b1ac0 to 4b83f2f13a556e1d28702b5cce21cfa57d3d4a92
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4b83f2f  Move matrices to new coercion model

comment:40 Changed 3 years ago by
 Dependencies changed from #25504, #25505, #25542, #25554, #25555 to #25504, #25505, #25542, #25554, #25555, #25871
comment:41 Changed 3 years ago by
 Commit changed from 4b83f2f13a556e1d28702b5cce21cfa57d3d4a92 to ecfb511e4375e7fd0bb1e4f222d028d48a8c4ba6
comment:42 Changed 3 years ago by
The error seems unrelated to this ticket. Somebody also got it because of the Singular upgrade, see #25969.
comment:43 Changed 3 years ago by
 Dependencies changed from #25504, #25505, #25542, #25554, #25555, #25871 to #25504, #25505, #25542, #25554, #25555, #25969
 Status changed from needs_work to positive_review
comment:44 Changed 3 years ago by
 Milestone changed from sage8.3 to sage8.4
update milestone 8.3 > 8.4
comment:45 Changed 3 years ago by
 Commit changed from ecfb511e4375e7fd0bb1e4f222d028d48a8c4ba6 to b03b8bf02a33a4cd5da4fa8e15d0e528f4efacdf
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:
b03b8bf  Move matrices to new coercion model

comment:46 followups: ↓ 47 ↓ 48 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:47 in reply to: ↑ 46 Changed 3 years ago by
comment:48 in reply to: ↑ 46 Changed 3 years ago by
comment:49 Changed 3 years ago by
I just tested again and this no longer causes #25969.
comment:50 Changed 3 years ago by
 Dependencies changed from #25504, #25505, #25542, #25554, #25555, #25969 to #25504, #25505, #25542, #25554, #25555
comment:51 Changed 3 years ago by
 Branch changed from u/jdemeyer/move_matrices_to_new_coercion_model to b03b8bf02a33a4cd5da4fa8e15d0e528f4efacdf
 Resolution set to fixed
 Status changed from positive_review to closed
If you want progress on this, it would be good to review #24742.