Fix Matrix_gfpn_dense * int

[simon-king-jena]

The following previous bug was fixed by #24742:

sage: M = matrix(GF(9,'x'), 2,3)
sage: type(M)
<type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
sage: M*1
[0 0 0]
[0 0 0]
sage: M*int(1)
...
ValueError: Cannot initialise non-square matrix from 1

This ticket adds a test that makes sure that the issue remains fixed.

Depends on #24742

CC: @tscrim

Component: coercion

Author: Simon King

Branch: f0e97af

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/25076

[simon-king-jena]

comment:1

While we are at it: How is one supposed to run optional tests? It seems that "sage -t --optional=meataxe" will ONLY run the tests marked as optional, hence, it will skip all compulsory tests, and thus will fail in most tests as definitions given in the compulsory tests are missing when running the optional tests.

[dimpase]

comment:2

Replying to @simon-king-jena:

While we are at it: How is one supposed to run optional tests? It seems that "sage -t --optional=meataxe" will ONLY run the tests marked as optional, hence, it will skip all compulsory tests, and thus will fail in most tests as definitions given in the compulsory tests are missing when running the optional tests.

sage -t --optional=meataxe,sage will do.

[jdemeyer]

comment:4

Works for me:

SageMath version 8.2.rc0, Release Date: 2018-03-28
sage: M = matrix(GF(9,'x'), 2,3); type(M)
<type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
sage: M * int(1)
[0 0 0]
[0 0 0]

[jdemeyer]

Replying to @simon-king-jena:

Crashboom
...
ValueError: Cannot initialise non-square matrix from 1

Please read https://groups.google.com/forum/#!topic/sage-devel/H0t9l6XdfPI

[jdemeyer]

comment:6

Simon: can you specify which version of Sage you using and whether any patches are applied?

[simon-king-jena]

comment:7

Replying to @jdemeyer:

Simon: can you specify which version of Sage you using and whether any patches are applied?

Sorry. It's the branch from #18514, which means Sage 8.5.beta2. Another change: I recently installed tkinter. And something seems to be broken anyway: "git status" gives me untracked files bin/, share/ etc. Likely reason: When I manually installed something, I gave the wrong prefix (SAGE_ROOT instead of SAGE_LOCAL) during configuration.

After removing the misplaced directories, re-installing the packages and sage -br, I get

sage: M = MatrixSpace(GF(9,'x'), 2,3)()
sage: type(M)
<type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
sage: M*int(1)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-3-1a2faedb318b> in <module>()
----> 1 M*int(Integer(1))

/home/king/Sage/git/sage/src/sage/structure/element.pyx in sage.structure.element.Matrix.__mul__ (build/cythonized/sage/structure/element.c:23460)()
   3671         if have_same_parent(left, right):
   3672             return (<Matrix>left)._matrix_times_matrix_(<Matrix>right)
-> 3673         return coercion_model.bin_op(left, right, mul)
   3674 
   3675     def __truediv__(left, right):

/home/king/Sage/git/sage/src/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:9671)()
   1111             if xp is yp:
   1112                 return op(x,y)
-> 1113             action = self.get_action(xp, yp, op, x, y)
   1114             if action is not None:
   1115                 return (<Action>action)._call_(x, y)

/home/king/Sage/git/sage/src/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.get_action (build/cythonized/sage/structure/coerce.c:16889)()
   1654         except KeyError:
   1655             pass
-> 1656         action = self.discover_action(R, S, op, r, s)
   1657         action = self.verify_action(action, R, S, op)
   1658         self._action_maps.set(R, S, op, action)

/home/king/Sage/git/sage/src/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel_cache_maps.discover_action (build/cythonized/sage/structure/coerce.c:18411)()
   1804 
   1805         if isinstance(R, Parent):
-> 1806             action = (<Parent>R).get_action(S, op, True, r, s)
   1807             if action is not None:
   1808                 return action

/home/king/Sage/git/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent.get_action (build/cythonized/sage/structure/parent.c:21664)()
   2493         action = self._get_action_(S, op, self_on_left)
   2494         if action is None:
-> 2495             action = self.discover_action(S, op, self_on_left, self_el, S_el)
   2496 
   2497         if action is not None:

/home/king/Sage/git/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent.discover_action (build/cythonized/sage/structure/parent.c:23017)()
   2604                     return action
   2605 
-> 2606                 if parent_is_integers(S) and not self.has_coerce_map_from(S):
   2607                     from sage.structure.coerce_actions import IntegerMulAction
   2608                     try:

/home/king/Sage/git/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent.has_coerce_map_from (build/cythonized/sage/structure/parent.c:17925)()
   2022                 print("Warning: non-unique parents %s" % (type(S)))
   2023             return True
-> 2024         return self._internal_coerce_map_from(S) is not None
   2025 
   2026     cpdef _coerce_map_from_(self, S):

/home/king/Sage/git/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent._internal_coerce_map_from (build/cythonized/sage/structure/parent.c:18944)()
   2164         try:
   2165             _register_pair(self, S, "coerce")
-> 2166             mor = self.discover_coerce_map_from(S)
   2167             #if mor is not None:
   2168             #    # Need to check that this morphism doesn't connect previously unconnected parts of the coercion diagram

/home/king/Sage/git/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent.discover_coerce_map_from (build/cythonized/sage/structure/parent.c:19396)()
   2301                 return (<Parent>S)._embedding
   2302 
-> 2303         user_provided_mor = self._coerce_map_from_(S)
   2304 
   2305         if user_provided_mor is None or user_provided_mor is False:

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent._coerce_map_from_ (build/cythonized/sage/structure/parent_old.c:7549)()
    339     cpdef _coerce_map_from_(self, S):
    340         if self._element_constructor is None:
--> 341             return self.coerce_map_from_c(S)
    342         else:
    343             return parent.Parent._coerce_map_from_(self, S)

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent.coerce_map_from_c (build/cythonized/sage/structure/parent_old.c:4044)()
    157                 self._coerce_from_hash[S] = None
    158                 return None
--> 159             mor = self.coerce_map_from_c(sage_type)
    160             if mor is not None:
    161                 mor = mor * sage_type._internal_coerce_map_from(S)

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent.coerce_map_from_c (build/cythonized/sage/structure/parent_old.c:3768)()
    141             pass
    142 
--> 143         mor = self.coerce_map_from_c_impl(S)
    144         import sage.categories.morphism
    145         import sage.categories.map

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent.coerce_map_from_c_impl (build/cythonized/sage/structure/parent_old.c:4710)()
    189         # Piggyback off the old code for now
    190         # WARNING: when working on this, make sure circular dependencies aren't introduced!
--> 191         if self.has_coerce_map_from_c(S):
    192             if isinstance(S, type):
    193                 S = Set_PythonType(S)

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent.has_coerce_map_from_c (build/cythonized/sage/structure/parent_old.c:6312)()
    275             except KeyError:
    276                 pass
--> 277         x = self.has_coerce_map_from_c_impl(S)
    278         self._has_coerce_map_from.set(S, x)
    279         return x

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent.has_coerce_map_from_c_impl (build/cythonized/sage/structure/parent_old.c:6489)()
    284             return False
    285         try:
--> 286             self._coerce_c((<parent.Parent>S).an_element())
    287         except TypeError:
    288             return False

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent._coerce_c (build/cythonized/sage/structure/parent_old.c:5382)()
    217             pass
    218         if HAS_DICTIONARY(self):
--> 219             return self._coerce_impl(x)
    220         else:
    221             return self._coerce_c_impl(x)

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/matrix/matrix_space.pyc in _coerce_impl(self, x)
   1100             and self.base_ring().has_coerce_map_from(x.base_ring())):
   1101             return self(x)
-> 1102         return self._coerce_try(x, self.base_ring())
   1103 
   1104     def _repr_(self):

/home/king/Sage/git/sage/src/sage/structure/parent_old.pyx in sage.structure.parent_old.Parent._coerce_try (build/cythonized/sage/structure/parent_old.c:5902)()
    255             try:
    256                 y = R._coerce_(x)
--> 257                 return self(y)
    258             except (TypeError, AttributeError) as msg:
    259                 pass

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/matrix/matrix_space.pyc in __call__(self, entries, coerce, copy, sparse)
    873             [t]
    874         """
--> 875         return self.matrix(entries, coerce, copy)
    876 
    877     def change_ring(self, R):

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/matrix/matrix_space.pyc in matrix(self, x, coerce, copy)
   1886                 x = list_to_dict(x, m, n)
   1887                 copy = False
-> 1888         return MC(self, x, copy=copy, coerce=coerce)
   1889 
   1890     def matrix_space(self, nrows=None, ncols=None, sparse=False):

/home/king/Sage/git/sage/src/sage/matrix/matrix_gfpn_dense.pyx in sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense.__init__ (build/cythonized/sage/matrix/matrix_gfpn_dense.c:5278)()
    423                 return
    424             if self._nrows != self._ncols:
--> 425                 raise ValueError("Cannot initialise non-square matrix from {}".format(data))
    426             f = FfFromInt(self._converter.field_to_int(self._coerce_element(data)))
    427             x = self.Data.Data

ValueError: Cannot initialise non-square matrix from 1

So, perhaps I should retry with #18514 rebased on top of the latest develop branch?

[simon-king-jena]

comment:8

PS: The same error was observed by other people as well, on #18514. So, I have to see what went wrong there.

[jdemeyer]

comment:9

Replying to @simon-king-jena:

So, perhaps I should retry with #18514 rebased on top of the latest develop branch?

Yes and let me know if that fixes it.

[simon-king-jena]

Attachment: giac-1.4.9.45.p2.log

Failing giac on Linux 4.4.0-116-generic #140-Ubuntu x86_64

[simon-king-jena]

comment:12

Replying to @jdemeyer:

Replying to @simon-king-jena:

So, perhaps I should retry with #18514 rebased on top of the latest develop branch?

Yes and let me know if that fixes it.

Giac fails to build (see attached log). Known problem?

[jdemeyer]

comment:13

Replying to @simon-king-jena:

Giac fails to build (see attached log). Known problem?

People have reported it but nobody knows how to reproduce it.

[simon-king-jena]

comment:14

Replying to @jdemeyer:

Replying to @simon-king-jena:

Giac fails to build (see attached log). Known problem?

People have reported it but nobody knows how to reproduce it.

Ouch. Any work-around (such as rebuilding from scratch)?

Could it be related with tkinter being installed (which used to not be the case when I last built Sage)?

[jdemeyer]

comment:15

Replying to @simon-king-jena:

such as rebuilding from scratch

It's certainly something to try. Please let us know whether that fixes your giac problem.

[simon-king-jena]

comment:16

I don't know why, but the two messages I was trying to send to sage-release via slrn didn't come through.

Trying now make distclean; make on a clean develop branch.

[simon-king-jena]

comment:17

OK, rebuilding from scratch did work. And with clean develop branch, I still get the error.

So, what is special about matrix_gfpn_dense? What is special about multiplying it with an int? And why the heck is _mul_long not called (why has it been removed from sage.matrix in the first place)?

[simon-king-jena]

comment:19

Now that's really frustrating. It is supposed to be related with coercion. Coercion is supposed to be machine independent. So, why (as Jeroen clarifies in the new ticket description) is the bug only present on some machines?

By the way, on #18514, at least one other person has seen that error. So, it is not restricted to my own laptop.

[jdemeyer]

comment:20

If it makes you feel better, I managed to reproduce it on a different machine.

[dimpase]

comment:21

Replying to @simon-king-jena:

OK, rebuilding from scratch did work. And with clean develop branch, I still get the error.

So, what is special about matrix_gfpn_dense? What is special about multiplying it with an int? And why the heck is _mul_long not called (why has it been removed from sage.matrix in the first place)?

did you try other native python types, e.g. float?

[dimpase]

comment:22

it might be gmp vs mpir difference, by the way.

[simon-king-jena]

comment:23

Replying to @dimpase:

Replying to @simon-king-jena:

OK, rebuilding from scratch did work. And with clean develop branch, I still get the error.

So, what is special about matrix_gfpn_dense? What is special about multiplying it with an int? And why the heck is _mul_long not called (why has it been removed from sage.matrix in the first place)?

did you try other native python types, e.g. float?

Doesn't make sense, as a float doesn't coerce into GF(3).

By inserting print statements into the bin_op method of the coercion model, I found that it isn't even called, although it should be.

[jdemeyer]

comment:37

Replying to @simon-king-jena:

What would be the point of closing this ticket as a duplicate (or maybe just adding a test) and then opening yet another ticket that deals with the two remaining issues?

1. It fixes the issue that Matrix * int didn't work.

2. It adds a doctest to show that 1. will remain fixed.

[simon-king-jena]

comment:38

Replying to @jdemeyer:

Replying to @simon-king-jena:

What would be the point of closing this ticket as a duplicate (or maybe just adding a test) and then opening yet another ticket that deals with the two remaining issues?

1. It fixes the issue that Matrix * int didn't work.

2. It adds a doctest to show that 1. will remain fixed.

Issue 1 is fixed in the dependency and moreover these are not the only issues related with the original ticket description. And what would be the point of opening yet another ticket when the remaining issue can be dealt with here?

It is, by the way, not without precedence that a ticket description was changed because of issues that came up while working on the original ticket description.

[jdemeyer]

comment:39

Replying to @simon-king-jena:

And what would be the point of opening yet another ticket when the remaining issue can be dealt with here?

Because this ticket already does something useful.

To turn your question around: what would be the point of not opening yet another ticket given that there are really two issues here (one about coercing scalars to the base ring and one about using _mul_long)?

[simon-king-jena]

Branch: u/SimonKing/fix_matrix_gfpn_dense___int

[simon-king-jena]

Changed keywords from _mul_long to none

[simon-king-jena]

Author: SimonKing

[simon-king-jena]

comment:41

OK, just adding a test here, and then opening another ticket for the two remaining issues (that do belong together).

---

New commits:

bf9cefd	New MatrixArgs object to deal with constructing matrices
cc82825	Merge branch 'u/jdemeyer/new_matrixinput_object_to_deal_with_constructing_matrices' of git://trac.sagemath.org/sage into t/25076/fix_matrix_gfpn_dense___int
bed63f0	Add a test ensuring that #24742 remains fixed

[simon-king-jena]

Commit: bed63f0

[simon-king-jena]

comment:42

Note that I constructed the test in such a way that blindly using the current implementation of _mul_long would give the wrong result, which means it also makes sure that the follow-up ticket will use the right conversion.

[jdemeyer]

comment:43

Replying to @simon-king-jena:

opening another ticket for the two remaining issues (that do belong together).

Those still look like different issues to me. One is about the coercion model and one is about the meataxe interface.

[simon-king-jena]

comment:44

Replying to @simon-king-jena:

Note that I constructed the test in such a way that blindly using the current implementation of _mul_long would give the wrong result, which means it also makes sure that the follow-up ticket will use the right conversion.

See #25079

[jdemeyer]

comment:45

I think it would be good to refer to this ticket instead of #24742 and to add a test for int * Matrix too.

[simon-king-jena]

comment:46

Replying to @jdemeyer:

Replying to @simon-king-jena:

opening another ticket for the two remaining issues (that do belong together).

Those still look like different issues to me. One is about the coercion model and one is about the meataxe interface.

Yes, they are two different issues, but it is totally natural to deal with them on a single ticket: When one only fixes the coercion model, one would actually introduce a bug (namely: The flawed _mul_long of Matrix_gfpn_dense would be used). When one only fixes the meataxe interface, one wouldn't be able to demonstrate that it is fixed without using cython in doctests.

Yes, using cython in doctests is possible, but why should one, if the existing test "M*int(4)==M" is good enough?

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f0e97af

Modify the test that ensures that #24742 remains fixed

[sagetrac-git]

Changed commit from bed63f0 to f0e97af

[simon-king-jena]

comment:48

Replying to @jdemeyer:

I think it would be good to refer to this ticket instead of #24742 and to add a test for int * Matrix too.

Done.

[jdemeyer]

Reviewer: Jeroen Demeyer

[vbraun]

Changed branch from u/SimonKing/fix_matrix_gfpn_dense___int to f0e97af

[jdemeyer]

Changed commit from f0e97af to none

[jdemeyer]

Changed author from SimonKing to Simon King