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: sage-8.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:

Status badges

Description (last modified by jdemeyer)

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 vdelecroix

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • 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 tscrim

  • Description modified (diff)
  • Milestone changed from sage-8.1 to sage-8.2

comment:4 Changed 3 years ago by jdemeyer

If you want progress on this, it would be good to review #24742.

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 3 years ago by jdemeyer

  • Dependencies #24742 deleted
  • Milestone changed from sage-8.2 to sage-8.3

comment:7 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

Working on this...

comment:8 Changed 3 years ago by jdemeyer

Vincent, since you created this ticket: are there any concrete issues that would be fixed by this?

comment:9 follow-up: Changed 3 years ago by vdelecroix

My aim was to allow matrices on semi-rings (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 jdemeyer

Replying to vdelecroix:

My aim was to allow matrices on semi-rings

What do you mean with "allow"? And how would that be related to changing element_class?

comment:11 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/move_matrices_to_new_coercion_model

comment:12 Changed 3 years ago by git

  • Commit set to e48d153eeee6cc6180abc61a96cae6e618784733

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ac7cf06Implement _an_element_ for matrix spaces
e48d153Move matrices to new coercion model

comment:13 Changed 3 years ago by jdemeyer

  • Dependencies set to #25504

comment:14 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25504 to #25504, #25505

comment:15 Changed 3 years ago by git

  • Commit changed from e48d153eeee6cc6180abc61a96cae6e618784733 to c646dc23a2a0306d03f358b4f1a6f65620805185

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

3c4a06dEnable _mul_long for matrices
1eaed37More stuff in the meataxe interface, and a meataxe helper function
8a06c0fFix docstring formatting
13da208Clean up creating Matrix_gfpn_dense matrices
cb52ee3Mark one doctest optional
c8fa7bbClean up __cinit__ methods of matrices
779dffbImplement _an_element_ for matrix spaces
6b8517fMerge commit '779dffbbb514290400d2b164b80ddb1a6c8ba04e' into t/23719/move_matrices_to_new_coercion_model
55568c8Misc matrix fixes
c646dc2Move matrices to new coercion model

comment:16 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25504, #25505 to #25504, #25505, #25542

comment:17 Changed 3 years ago by jdemeyer

  • Status changed from new to needs_review

comment:18 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Doctest failures

comment:19 Changed 3 years ago by git

  • Commit changed from c646dc23a2a0306d03f358b4f1a6f65620805185 to 8769af1d2248e8193ac95007adcb038df46a3ef0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b3901d7Misc matrix fixes
c8ce255Merge commit 'c8fa7bb27f4724a7263d76bec437ebcf7f87dec3'; commit 'b3901d7dea1ac4bcf7b8c4e2d7adf709b92536cd' into t/23719/move_matrices_to_new_coercion_model
8769af1Move matrices to new coercion model

comment:20 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25504, #25505, #25542 to #25504, #25505, #25542, #25554

comment:21 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25504, #25505, #25542, #25554 to #25504, #25505, #25542, #25554, #25555

comment:22 Changed 3 years ago by git

  • Commit changed from 8769af1d2248e8193ac95007adcb038df46a3ef0 to b6fd971e30aa6a7504e3513905dc82ee249ae74f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

154dbd5Replace _coerce_ checks by has_coerce_map_from
15f31c0Support new-style Parents in RingMap_lift
c821441Merge commit 'c8fa7bb27f4724a7263d76bec437ebcf7f87dec3'; commit 'b3901d7dea1ac4bcf7b8c4e2d7adf709b92536cd'; commit '154dbd5f68f71bd64855093da869debdbdda1925'; commit '15f31c09ce3973845f2bff779f43bf924be83f14' into t/23719/move_matrices_to_new_coercion_model
b6fd971Move matrices to new coercion model

comment:23 Changed 3 years ago by git

  • Commit changed from b6fd971e30aa6a7504e3513905dc82ee249ae74f to 4fa13cdd8c3ed1551eec834362fe467a229ff339

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e1bb9b6Replace _coerce_ checks by has_coerce_map_from
0b37559Merge commit '15f31c09ce3973845f2bff779f43bf924be83f14'; commit 'c8fa7bb27f4724a7263d76bec437ebcf7f87dec3'; commit 'b3901d7dea1ac4bcf7b8c4e2d7adf709b92536cd'; commit 'e1bb9b67354593d944aa38864b55112fbdedfec5' into t/23719/move_matrices_to_new_coercion_model
4fa13cdMove matrices to new coercion model

comment:24 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:25 Changed 3 years ago by jdemeyer

All tests passed!

comment:26 follow-up: Changed 3 years ago by vdelecroix

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 jdemeyer

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 jdemeyer

I just realized that this hasn't been reviewed yet...

Ping?

comment:29 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Oops, merge failure...

comment:30 Changed 3 years ago by git

  • Commit changed from 4fa13cdd8c3ed1551eec834362fe467a229ff339 to 4e76683904bd815de104fac43251d35fddfd5193

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4e76683Move matrices to new coercion model

comment:31 Changed 3 years ago by git

  • Commit changed from 4e76683904bd815de104fac43251d35fddfd5193 to 5c6e09ebe03308a2b6141957046eb5e083e7c87b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5c6e09eMove matrices to new coercion model

comment:32 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:33 Changed 3 years ago by git

  • Commit changed from 5c6e09ebe03308a2b6141957046eb5e083e7c87b to 0ae7a1c0187769dfadee78e1ad6d8190eceea909

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0ae7a1cMove matrices to new coercion model

comment:34 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

LGTM assuming a patchbot comes back clean. Vincent, any other comments?

comment:35 Changed 3 years ago by git

  • Commit changed from 0ae7a1c0187769dfadee78e1ad6d8190eceea909 to 9715f55d97776b31e64d96e356291f12971b1ac0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9715f55Move matrices to new coercion model

comment:36 Changed 3 years ago by jdemeyer

  • Keywords days94 added

comment:37 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:38 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

I'm getting this error when running the testsuite on 32-bit 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/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/sage/schemes/elliptic_curves/padics.py", line 1095, in padic_sigma
        E2 = self.padic_E2(p, N-2, check_hypotheses=False)
      File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/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 git

  • Commit changed from 9715f55d97776b31e64d96e356291f12971b1ac0 to 4b83f2f13a556e1d28702b5cce21cfa57d3d4a92

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4b83f2fMove matrices to new coercion model

comment:40 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25504, #25505, #25542, #25554, #25555 to #25504, #25505, #25542, #25554, #25555, #25871

I can confirm 38 in the sense that I can reproduce the error. I don't see how it could be caused by this ticket though. It might be related to GC for example where a different internal state causes a GC at a different time. To check that, I'm rebasing to #25871.

comment:41 Changed 3 years ago by git

  • Commit changed from 4b83f2f13a556e1d28702b5cce21cfa57d3d4a92 to ecfb511e4375e7fd0bb1e4f222d028d48a8c4ba6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ae560ceAdd option to control GC during doctests
ecfb511Move matrices to new coercion model

comment:42 Changed 3 years ago by jdemeyer

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 jdemeyer

  • 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 vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:45 Changed 3 years ago by git

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

b03b8bfMove matrices to new coercion model

comment:46 follow-ups: Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Is #25969 still a dependency? It seems from the comments like #25969 is not coming from this ticket.

comment:47 in reply to: ↑ 46 Changed 3 years ago by jdemeyer

Replying to tscrim:

Is #25969 still a dependency?

I don't know. We can test again whether the problem magically went away.

comment:48 in reply to: ↑ 46 Changed 3 years ago by jdemeyer

Replying to tscrim:

It seems from the comments like #25969 is not coming from this ticket.

Well, define "coming from". The problem is certainly unrelated and not directly caused by this ticket, but merging this ticket did cause #25969 to appear.

comment:49 Changed 3 years ago by jdemeyer

I just tested again and this no longer causes #25969.

comment:50 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25504, #25505, #25542, #25554, #25555, #25969 to #25504, #25505, #25542, #25554, #25555

comment:51 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/move_matrices_to_new_coercion_model to b03b8bf02a33a4cd5da4fa8e15d0e528f4efacdf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.