[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0 of 4] Implement basic sparse op diag and diag op sparse su
From: |
Jaroslav Hajek |
Subject: |
Re: [PATCH 0 of 4] Implement basic sparse op diag and diag op sparse support. |
Date: |
Tue, 10 Mar 2009 07:37:10 +0100 |
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
- [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, (continued)
- [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, Jason Riedy, 2009/03/09
- [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, John W. Eaton, 2009/03/09
- Re: [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, Jason Riedy, 2009/03/10
- Re: [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, John W. Eaton, 2009/03/10
- Re: [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, Jason Riedy, 2009/03/10
- Re: [PATCH 4 of 4] Implement diag + sparse, diag - sparse, sparse + diag, sparse - diag, Jaroslav Hajek, 2009/03/11
- [PATCH 0 of 4] Implement basic sparse op diag and diag op sparse support., John W. Eaton, 2009/03/09
- Re: [PATCH 0 of 4] Implement basic sparse op diag and diag op sparse support.,
Jaroslav Hajek <=
- Re: [PATCH 0 of 4] Implement basic sparse op diag and diag op sparse support., Jaroslav Hajek, 2009/03/12