Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#25076 closed defect (fixed)

Fix Matrix_gfpn_dense * int

Reported by: SimonKing Owned by:
Priority: critical Milestone: sage-8.2
Component: coercion Keywords:
Cc: tscrim Merged in:
Authors: Simon King Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: f0e97af (Commits) Commit:
Dependencies: #24742 Stopgaps:

Description (last modified by SimonKing)

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.

Attachments (1)

giac-1.4.9.45.p2.log (113.2 KB) - added by SimonKing 3 years ago.
Failing giac on Linux 4.4.0-116-generic #140-Ubuntu x86_64

Download all attachments as: .zip

Change History (52)

comment:1 follow-up: Changed 3 years ago by SimonKing

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.

comment:2 in reply to: ↑ 1 Changed 3 years ago by dimpase

Replying to SimonKing:

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.

comment:3 Changed 3 years ago by tscrim

  • Cc tscrim added

comment:4 Changed 3 years ago by jdemeyer

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]

comment:5 in reply to: ↑ description Changed 3 years ago by jdemeyer

Replying to SimonKing:

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

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

comment:6 follow-up: Changed 3 years ago by jdemeyer

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

comment:7 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by SimonKing

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?

comment:8 Changed 3 years ago by SimonKing

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

comment:9 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

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

Yes and let me know if that fixes it.

comment:10 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 3 years ago by SimonKing

  • Description modified (diff)

Changed 3 years ago by SimonKing

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

comment:12 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

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?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

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

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

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)?

comment:15 in reply to: ↑ 14 Changed 3 years ago by jdemeyer

Replying to SimonKing:

such as rebuilding from scratch

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

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

comment:16 Changed 3 years ago by SimonKing

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.

comment:17 follow-up: Changed 3 years ago by SimonKing

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)?

comment:18 Changed 3 years ago by jdemeyer

  • Component changed from packages: optional to coercion
  • Description modified (diff)
  • Summary changed from Un-break optional meataxe backend to Fix Matrix_gfpn_dense * int

comment:19 Changed 3 years ago by SimonKing

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.

comment:20 Changed 3 years ago by jdemeyer

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

comment:21 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by dimpase

Replying to SimonKing:

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?

comment:22 follow-ups: Changed 3 years ago by dimpase

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

comment:23 in reply to: ↑ 21 Changed 3 years ago by SimonKing

Replying to dimpase:

Replying to SimonKing:

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.

comment:24 in reply to: ↑ 22 Changed 3 years ago by SimonKing

Replying to dimpase:

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

Does it even come that far?

comment:25 Changed 3 years ago by SimonKing

Or to be precise: The bin_op method *is* called, but it doesn't come to the line "if op is mul or op is imul:..."

comment:26 Changed 3 years ago by jdemeyer

This seems to be fixed (accidentally) by #24742.

Proposal: consider this issue a duplicate, just add a doctest to prevent regressions.

comment:27 in reply to: ↑ 22 ; follow-up: Changed 3 years ago by SimonKing

Replying to dimpase:

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

I just noticed something that supports your guess. When doing "sage -br", I noticed these lines:

Updating Cython code....
sage/rings/complex_double.pyx: cannot find cimported module 'gmpy2'
sage/rings/complex_number.pyx: cannot find cimported module 'gmpy2'
sage/rings/integer.pyx: cannot find cimported module 'gmpy2'
sage/rings/complex_mpc.pyx: cannot find cimported module 'gmpy2'
sage/rings/rational.pyx: cannot find cimported module 'gmpy2'
sage/rings/real_double.pyx: cannot find cimported module 'gmpy2'
sage/rings/real_mpfr.pyx: cannot find cimported module 'gmpy2'

comment:28 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Dependencies set to #24742

comment:29 in reply to: ↑ 27 Changed 3 years ago by jdemeyer

Replying to SimonKing:

sage/rings/complex_double.pyx: cannot find cimported module 'gmpy2'
sage/rings/complex_number.pyx: cannot find cimported module 'gmpy2'
sage/rings/integer.pyx: cannot find cimported module 'gmpy2'
sage/rings/complex_mpc.pyx: cannot find cimported module 'gmpy2'
sage/rings/rational.pyx: cannot find cimported module 'gmpy2'
sage/rings/real_double.pyx: cannot find cimported module 'gmpy2'
sage/rings/real_mpfr.pyx: cannot find cimported module 'gmpy2'

Red herring. Those warnings only mean that the Python package gmpy2 was not installed.

comment:30 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_matrix_gfpn_dense___int

comment:31 Changed 3 years ago by jdemeyer

  • Commit set to 59ac78a4cf1f5c103b2e1725c52df27d6b264eb1
  • Status changed from new to needs_review

New commits:

bf9cefdNew MatrixArgs object to deal with constructing matrices
59ac78aAdd doctest for Trac #25076

comment:32 follow-ups: Changed 3 years ago by SimonKing

  • Status changed from needs_review to needs_work

It isn't fixed. I inserted a print statement in the _mul_long method of matrix_gfpn_dense, but apparently the method is not called.

By consequence, the Python int is converted to an element of the base ring and then multiplied using _lmul_. I should run some timings, but I am sure that this is slower than using _mul_long.

comment:33 in reply to: ↑ 32 Changed 3 years ago by SimonKing

Replying to SimonKing:

It isn't fixed. I inserted a print statement in the _mul_long method of matrix_gfpn_dense, but apparently the method is not called.

By consequence, the Python int is converted to an element of the base ring and then multiplied using _lmul_. I should run some timings, but I am sure that this is slower than using _mul_long.

Actually it is a lot worse than I thought:

sage: M = random_matrix(GF(9,'x'), 64,51)
sage: type(M)
<type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
sage: cython("""
....: from sage.matrix.matrix_gfpn_dense cimport Matrix_gfpn_dense as MTX
....: def test1(MTX M):
....:     cdef size_t i
....:     cdef MTX N
....:     for i in range(100000):
....:         N = M*i
....: def test2(MTX M):
....:     cdef size_t i
....:     cdef MTX N
....:     for i in range(100000):
....:         N = M._mul_long(i)
....: """)
sage: %timeit test1(M)
1 loop, best of 3: 11.7 s per loop
sage: %timeit test2(M)
1 loop, best of 3: 366 ms per loop
sage: 11.7/0.366
31.9672131147541

So, the current code is almost 32 times slower than what used to happen, in a scenario that I do care about.

Hence, adding a doctest isn't enough.

comment:34 in reply to: ↑ 32 ; follow-up: Changed 3 years ago by jdemeyer

  • Authors Jeroen Demeyer deleted
  • Branch u/jdemeyer/fix_matrix_gfpn_dense___int deleted
  • Commit 59ac78a4cf1f5c103b2e1725c52df27d6b264eb1 deleted
  • Dependencies #24742 deleted

Replying to SimonKing:

It isn't fixed.

I just pushed a branch fixing exactly the problem that you described and you're not happy because it doesn't also fix another issue? Good luck fixing it your way then.

comment:35 in reply to: ↑ 34 ; follow-up: Changed 3 years ago by SimonKing

  • Dependencies set to #24742
  • Description modified (diff)
  • Keywords _mul_long added; meataxe coercion removed

Replying to jdemeyer:

Replying to SimonKing:

It isn't fixed.

I just pushed a branch fixing exactly the problem that you described and you're not happy because it doesn't also fix another issue? Good luck fixing it your way then.

Your behaviour makes me speechless. Almost.

The ticket description is about using the short-cut _mul_long as a solution to the problem that Matrix*int didn't work. As it turns out #24742 allows Matrix*int, but it doesn't do it by enabling _mul_long (although _mul_long is supported in sage.structure.element).

So, given the poor performance I demonstrated, there is an issue here that goes beyond what was fixed in #24742, and it is related with what the ticket description of this ticket says.

In addition, I notice that the current implementation of _mul_long for Matrix_gfpn_dense doesn't convert an integer into GF(p^n,'x') by reduction modulo p, but by some conversion used by MeatAxe. For instance, 4 would be converted to the number x+1 in GF(9,'x'). I didn't notice it before since in my cohomology applications I work with matrices over prime fields, where the different conversions happen to coincide.

Hence, if this ticket is about to "Fix Matrix_gfpn_dense*int", then here one should

  1. re-enable the usage of _mul_long for matrix times int, because of the performance issue
  2. fix _mul_long of Matrix_gfpn_dense for non-prime fields.

I changed the ticket description accordingly.

Consequently, this ticket is by no means just a duplicate of #24742.

And even when you insist that it is a duplicate: 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?

Alternatively, I could just leave the issues alone and use ._mul_long directly in my cohomology code, to be independent of whatever implementation of coercion is currently used.

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

comment:36 Changed 3 years ago by SimonKing

  • Description modified (diff)

comment:37 in reply to: ↑ 35 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

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.
  1. It adds a doctest to show that 1. will remain fixed.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

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.
  1. 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.

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

comment:39 in reply to: ↑ 38 Changed 3 years ago by jdemeyer

Replying to SimonKing:

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)?

comment:40 Changed 3 years ago by SimonKing

  • Branch set to u/SimonKing/fix_matrix_gfpn_dense___int

comment:41 follow-up: Changed 3 years ago by SimonKing

  • Authors set to SimonKing
  • Commit set to bed63f0a555cb1b8f291ec65428c0cf972fd40f8
  • Description modified (diff)
  • Keywords _mul_long removed
  • Status changed from needs_work to needs_review

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


New commits:

bf9cefdNew MatrixArgs object to deal with constructing matrices
cc82825Merge 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
bed63f0Add a test ensuring that #24742 remains fixed

comment:42 follow-up: Changed 3 years ago by SimonKing

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.

comment:43 in reply to: ↑ 41 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

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.

comment:44 in reply to: ↑ 42 Changed 3 years ago by SimonKing

Replying to SimonKing:

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

comment:45 follow-up: Changed 3 years ago by jdemeyer

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

comment:46 in reply to: ↑ 43 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

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?

comment:47 Changed 3 years ago by git

  • Commit changed from bed63f0a555cb1b8f291ec65428c0cf972fd40f8 to f0e97af5a3dbffec014829eb2341abdaecb78e9f

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

f0e97afModify the test that ensures that #24742 remains fixed

comment:48 in reply to: ↑ 45 Changed 3 years ago by SimonKing

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.

comment:49 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:50 Changed 3 years ago by vbraun

  • Branch changed from u/SimonKing/fix_matrix_gfpn_dense___int to f0e97af5a3dbffec014829eb2341abdaecb78e9f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:51 Changed 2 years ago by jdemeyer

  • Authors changed from SimonKing to Simon King
  • Commit f0e97af5a3dbffec014829eb2341abdaecb78e9f deleted
Note: See TracTickets for help on using tickets.