Re: [PATCH 0 of 4] Implement basic sparse op diag and diag op sparse support.

[Jaroslav Hajek]

On Mon, Mar 9, 2009 at 10:52 PM, Jason Riedy <address@hidden> wrote:
> Only *, /, \, +, and - are implemented.  Those are the ones that would be 
> nicest to have, at
> least for me.  Because the support is limited to those operations, I avoided 
> serious macro
> hackery and most of the auto-generation mechanism.  It's overkill here.
>
> I'll poke at permutation matrices next.  I have code that works, but I need 
> to clean it up
> considerably.
>
> I'd love to see this considered for 3.2.  Having eye (N) * A stay sparse 
> would be great,
> as would regularization_factor * eye (N) + A.  Code would be much more 
> readable.  The only
> possible downside would be if someone is relying on multiplication by eye () 
> to make a
> sparse matrix dense.
>

Yes, it certainly would be more logical and likely more useful.
However, I don't see this as a trivial change and I don't think all
issues have been addressed (see below). Getting this into 3.2 would
delay the release, which I think is not good, especially when it seems
that 3.0.x will have no more releases.

> Jason
>

I tried all patches in a test repo - all applied smoothly. After a
short inspection, I have the following comments:

1. I'm not sure we need a separate "octave_impl" namespace. I was
hoping that just "octave" will suit well. Besides, the transition
should probably happen in an organized manner.

2. I see you didn't reuse the existing mechanism Sparse-op-defs.h /
sparse-mx-ops / sparse-mk-ops.awk.
OK, the practice of putting executive code into macros is not really
nice, but I think you could have just create the necessary macros as
wrappers over the templates do_mul_dm_sm etc.
I agree we need to improve design of liboctave after 3.2, use more
templates and less macros, especially for executive code (which is
very difficult to debug in macros), but I think it should be organized
to avoid making more mess than necessary (I mean, more mess than there
is now).

3. You incorrectly implement the compound trans_mul and herm_mul
operations. these correspond to A.'*B and A'*B - and the diagonal
matrix must be transposed or conjugate-transposed. Remember that a
diagonal matrix may be rectangular. See DiagMatrix::transpose() etc.
These are not necessary - when the interpreter can't find a
specialized handler for the operation, it will just decompose it to a
transpose and multiplication. Unless you want a smarter
implementation, I think you can just omit those.
Note that transpose (not conjugate transpose) of a diagonal matrix
needs not copy the diagonal data.

4. in the OPERATORS/ implementation, I don't think there's any need to
check for single-element diagonal and sparse matrices in operations. A
single element sparse or diagonal matrix will be narrowed to a scalar
automatically (it happens when octave_values are created and in a few
other places), and if you force it to stay in matrix form, there's
really no need for it to work as a scalar. Also, I don't understand
why you need to explicitly set matrix_type for the return value. I
think the operators can just be simple wrappers over the liboctave
operations, implemented using DEFBINOP_OP.
I hoped op-dm-template.cc can be reused for that purpose.

Sorry, but I don't see these changes as mature enough to get into 3.2.

-- 
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz