Re: [sage-devel] Re: Weird issue with matrix mutability
I agree, the best is to speed up matrices at all, instead of creating a dangerous hack, just to speed up 2x2 Matrices. Further I think it's natural to inherit (most) matrix classes from Matrix, so this should not be changed. bg, Johannes On 11.01.2013 21:47, Nils Bruin wrote: > sage/matrix$ grep _mutability *.pxd > matrix0.pxd:cdef sage.structure.mutability.Mutability _mutability > matrix0.pxd:cdef check_mutability(self) > matrix0.pxd:cdef check_bounds_and_mutability(self, Py_ssize_t i, > Py_ssize_t j) > matrix0.pxd:cdef check_row_bounds_and_mutability(self, Py_ssize_t > r1, Py_ssize_t r2) > matrix0.pxd:cdef check_column_bounds_and_mutability(self, > Py_ssize_t c1, Py_ssize_t c2) > > so if you change that to > > cdef bint _mutability > > you probably only have to amend those 4 routines. Further attribute > access: > > sage/matrix$ grep \\._mutability *.pyx | wc > 20 691235 > > in matrix0.pyx these are just the lines in the functions above and the > remaining 9 lines are in a handful of special matrix implementations > and of the form > >if self._mutability._is_immutable: > > which aren't really adhering to protocol (accessing private > attributes! twice!), but are easily rewritten (to faster code anyway!) > > I thinks it's entirely doable to just make _mutability a straight > boolean flag. Does anyone have reasons why this is not a good idea? > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
Op vrijdag 11 januari 2013 14:27:43 UTC+1 schreef David Loeffler het volgende: > > I'm keen to keep Matrix_integer_2x2 as a subclass of Matrix. I'd have said > that doing it any other way is likely to cause lots of obscure bugs (since > developers will quite naturally assume that Matrix_xxx is a subclass of > Matrix for any value of xxx, and have probably already done so in existing > code). > > But not implementing it correctly will likely give similarly obscure bugs, don't you agree? Of course, those would probably be obscure bugs in future code instead of in existing code. Those developers who have already assumed that it is a subclass can't have used that assumption much, because it is a broken subclass implementation, breaking its superclass's methods. Note that I'm not against implementing the same interface, just against doing so by subclassing. Thanks to duck typing, that shouldn't break existing code that doesn't deserve it / that doesn't depend on a broken implementation. As you said earlier, this is not an end-user class, so not breaking existing code does not have as high a priority as it would otherwise have. Timo -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
I'm keen to keep Matrix_integer_2x2 as a subclass of Matrix. I'd have said that doing it any other way is likely to cause lots of obscure bugs (since developers will quite naturally assume that Matrix_xxx is a subclass of Matrix for any value of xxx, and have probably already done so in existing code). David On 11 January 2013 13:14, Timo Kluck wrote: > Op vrijdag 11 januari 2013 13:41:42 UTC+1 schreef David Loeffler het > volgende: >> >> >> > I think the reason for subclassing in this case is code-reuse. But >> that's in itself a dangerous thing to do if we change the class' internal > >> behavior by this much. People changing the code for Matrix_dense shouldn't >> need to deal with issues in a non-conforming subclass -- >> > but they will have to when their changes suddenly cause doctests to >> fail. >> >> The probelm with that is that conforming properly to the interface (...) >> results in slowing down 2x2 matrix multiplication by a factor of about 5. >> > > I understand that. That's why I'm suggesting not subclassing at all. Then > you can have all the speed benefit you want without breaking expectations. > As I said, when you don't conform to the interface, the only reason for > subclassing is code reuse, but that too is a shady thing to do when you > change the internals. > > I have a suggestion for a workaround: create a single Mutability object >> which is constant over each Sage session and is set to immutable, and >> assign this to the _mutability slot in _new_c. This means that we still >> preserve most of the speed advantage of the 2x2 integer matrix class, but >> it still conforms to the specification modulo the fact that it's not >> possible to create a mutable Matrix_integer_2x2. The downside is that we'd >> potentially end up with a situation where some Matrix_integer_2x2 objects >> will have "their own" Mutability objects, and some will have a pointer to a >> single global Mutability object, which is a bit mind-bending; but that >> doesn't matter since we will never change the global 2x2 matrix Mutability >> object anyway. >> >> This may be an adequate solution. I prefer not subclassing, though. How > do you feel about that? > > Timo > > -- > You received this message because you are subscribed to the Google Groups > "sage-devel" group. > To post to this group, send email to sage-devel@googlegroups.com. > To unsubscribe from this group, send email to > sage-devel+unsubscr...@googlegroups.com. > Visit this group at http://groups.google.com/group/sage-devel?hl=en. > > > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
Op vrijdag 11 januari 2013 13:41:42 UTC+1 schreef David Loeffler het volgende: > > > > I think the reason for subclassing in this case is code-reuse. But > that's in itself a dangerous thing to do if we change the class' internal > > behavior by this much. People changing the code for Matrix_dense shouldn't > need to deal with issues in a non-conforming subclass -- > > but they will have to when their changes suddenly cause doctests to fail. > > The probelm with that is that conforming properly to the interface (...) > results in slowing down 2x2 matrix multiplication by a factor of about 5. > I understand that. That's why I'm suggesting not subclassing at all. Then you can have all the speed benefit you want without breaking expectations. As I said, when you don't conform to the interface, the only reason for subclassing is code reuse, but that too is a shady thing to do when you change the internals. I have a suggestion for a workaround: create a single Mutability object > which is constant over each Sage session and is set to immutable, and > assign this to the _mutability slot in _new_c. This means that we still > preserve most of the speed advantage of the 2x2 integer matrix class, but > it still conforms to the specification modulo the fact that it's not > possible to create a mutable Matrix_integer_2x2. The downside is that we'd > potentially end up with a situation where some Matrix_integer_2x2 objects > will have "their own" Mutability objects, and some will have a pointer to a > single global Mutability object, which is a bit mind-bending; but that > doesn't matter since we will never change the global 2x2 matrix Mutability > object anyway. > > This may be an adequate solution. I prefer not subclassing, though. How do you feel about that? Timo -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
> I think the reason for subclassing in this case is code-reuse. But that's in itself a dangerous thing to do if we change the class' internal > behavior by this much. People changing the code for Matrix_dense shouldn't need to deal with issues in a non-conforming subclass -- > but they will have to when their changes suddenly cause doctests to fail. The probelm with that is that conforming properly to the interface (by creating a new Mutability object every time we instantiate a 2x2 integer matrix) results in slowing down 2x2 matrix multiplication by a factor of about 5. Since the special 2x2 matrix class is about 10 times faster for multiplication than the generic Matrix_integer_dense, this would almost totally destroy the point of having a special-case class for the 2x2 case in the first place. I have a suggestion for a workaround: create a single Mutability object which is constant over each Sage session and is set to immutable, and assign this to the _mutability slot in _new_c. This means that we still preserve most of the speed advantage of the 2x2 integer matrix class, but it still conforms to the specification modulo the fact that it's not possible to create a mutable Matrix_integer_2x2. The downside is that we'd potentially end up with a situation where some Matrix_integer_2x2 objects will have "their own" Mutability objects, and some will have a pointer to a single global Mutability object, which is a bit mind-bending; but that doesn't matter since we will never change the global 2x2 matrix Mutability object anyway. David -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
Op vrijdag 11 januari 2013 11:16:07 UTC+1 schreef David Loeffler het volgende: > > > So I think it's acceptable if Matrix_integer_2x2 doesn't implement > everything in the Sage matrix specification, > I almost agree. I think that it would be acceptable to raise a NotImplementedError for some methods, but random failures like the one we saw above should still be avoided. > and in particular if we set it up so its elements are *always* immutable; > and we change the .matrix() element of SL2Z elements to return a > Matrix_integer_dense copy of the internal Matrix_integer_2x2. What do > others think about this suggestion? > I'm not opposed to this. But if we change the behavior by that much, does it need to inherit from Matrix_dense at all? From my point of view, it would be better to not violate reasonable expectations people will have about the interface of subclasses, even if the one having the expectations is a 'developer', not an 'end-user'. I think the reason for subclassing in this case is code-reuse. But that's in itself a dangerous thing to do if we change the class' internal behavior by this much. People changing the code for Matrix_dense shouldn't need to deal with issues in a non-conforming subclass -- but they will have to when their changes suddenly cause doctests to fail. Timo -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
On 11 January 2013 09:29, Johhannes wrote: > > > Am Freitag, 11. Januar 2013 00:53:08 UTC+1 schrieb Nils Bruin: > >> >> >> It could be more appropriate to fix this by making 2x2 matrices always >> immutable (then the methods accessing that can be special-cases on the >> class) or by lying and just making set_immutable a no-op. >> > > this could be one way, but maybe some other functionallity gets broken > then. > >> >> Or perhaps by documenting "2x2 matrices aren't a full implementation >> of matrices, but we inherit from it anyway by way of a hack. Don't >> expect that ... will always properly work. Only use 2x2 matrices when >> you need the speed and don't care about the unimplemented features". >> > > I think as long as we inherit from one object, we should implement all > base methods. An 'end-user'-class should not have unimplemented functions. > > the best would be, to do some timing tests, in order to see how much > adding this line affects the performance. > The class Matrix_integer_2x2 isn't really an "end-user" class. If a user does sage: m = matrix(ZZ, 2, [1,2,3,4]) what they get is an instance of Matrix_integer_dense which happens to have size 2x2, not of Matrix_integer_2x2. The latter exists *solely* in order that it can get used by the modular forms code which does a lot of arithmetic with 2x2 integer matrices; arithmetic operations are a great deal faster with Matrix_integer_2x2 (a factor of 10 or so). The only way users can explicitly get their hands on a Matrix_integer_2x2 in a Sage session would be either to import the class constructor (which is not in the default Sage namespace) or to use the .matrix() object of SL2Z elements; the class sage.modular.arithgroup.arithgroup_element.ArithmeticSubgroupElement is a wrapper around Matrix_integer_2x2. So I think it's acceptable if Matrix_integer_2x2 doesn't implement everything in the Sage matrix specification, and in particular if we set it up so its elements are *always* immutable; and we change the .matrix() element of SL2Z elements to return a Matrix_integer_dense copy of the internal Matrix_integer_2x2. What do others think about this suggestion? David -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
I think it would be enough to move the _mutability = Mutability(False) statement from the _copy_() method to the _new_c() method. On 11.01.2013 00:20, Johannes wrote: > But in the _copy_() method _mutability is set to a value. Even if I > think this should be done in _new_c(). > > By the way, this bug also appears for x + y and x - y. > > greatz Johannes > > On 10.01.2013 23:34, Nils Bruin wrote: >> On Jan 10, 12:55 pm, Johannes wrote: >>> hey list, >>> >>> If you look at the code, there is a function called _new_c() which is >>> used in the multiplication algorithm. >> >> See >> >> http://trac.sagemath.org/sage_trac/ticket/8294 >> >> Looks like a little more is broken than just __copy__. >> >> Normally, _mutability gets set in matrix0's Matrix.__init__. >> >> Matrix_integer_dense gets that by explicitly calling a superclass >> __init__ in its __cinit__. I guess 2x2 tries to avoid that for >> efficiency reasons, but it suggests that its __cinit__ (or at least >> its _new_c) should then set the _mutability attribute. Is this class >> intentionally producing maimed instances for efficiency reasons >> perhaps? >> > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
Re: [sage-devel] Re: Weird issue with matrix mutability
But in the _copy_() method _mutability is set to a value. Even if I think this should be done in _new_c(). By the way, this bug also appears for x + y and x - y. greatz Johannes On 10.01.2013 23:34, Nils Bruin wrote: > On Jan 10, 12:55 pm, Johannes wrote: >> hey list, >> >> If you look at the code, there is a function called _new_c() which is >> used in the multiplication algorithm. > > See > > http://trac.sagemath.org/sage_trac/ticket/8294 > > Looks like a little more is broken than just __copy__. > > Normally, _mutability gets set in matrix0's Matrix.__init__. > > Matrix_integer_dense gets that by explicitly calling a superclass > __init__ in its __cinit__. I guess 2x2 tries to avoid that for > efficiency reasons, but it suggests that its __cinit__ (or at least > its _new_c) should then set the _mutability attribute. Is this class > intentionally producing maimed instances for efficiency reasons > perhaps? > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To post to this group, send email to sage-devel@googlegroups.com. To unsubscribe from this group, send email to sage-devel+unsubscr...@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel?hl=en.
