Re: [Lapackpp-devel] Error in LaGenMatDouble::copy(...) and other questions

[Christian S. <sti...@tu...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dominik Wagenfuehr schrieb:
> So, I have attached the three files for LaSymmBandMatDouble and
> LaSymmBandFactDouble. I hope this mailing list accept attachments...

You didn't send it to the mailing list, only to me directly. In the
future please make sure you always send a CC to the mailing list (or
vice versa, but I still suggest sending to both, see [1]).

> Most of the sybmd.h-parts are original. I have only commented them
> properly. There was a copy-Functon in sybmd.cc but however I've only got
> an error if I remove the "inline" from sybmd.h as you suggested the last
> time. So I deleted this copy-function (It was wrong anyway!) and add a
> new one in sybmd.h. Further the programmer forgot to reset N_ and kl_
> when copying or referencing another matrix.

Ok, thanks for fixing the missing parts. Some remarks:

* Please always use the code from CVS, now that you're actually working
with it. I already did some changes. This time I did the merging myself,
but in the future it is expected that any patches are against the
up-to-date CVS, not any released versions (which might already be
outdated). See [2] for CVS.

* As for inline vs. non-inline: I want to reduce the "inline" functions
to a minimum, because they contradict the OO-paradigm of "hiding the
implementation" (and make debugging harder, among other things).
Therefore if I review a class, I want to keep only those methods
"inline" where it actually makes sense. Inlining a function makes sense
if and only if the execution cost of the function is on the order of the
cost of the function call. OTOH if the cost of the function call is
negligible compared to the execution cost of the function, then inlining
is complete nonsense. Most prominent example here is copy(), which
obviously has a large execution cost, so there is no point at all in
marking this as inline. Same for any constructor. According to this
criterion I've moved many functions to the .cc file, and I've moved the
operator() to the header file, because this one is the counterexample
where inlining *does* make sense. Also, having the operator() inline
enables the client application to decide on the amount of error checking
by defining or not defining LA_BOUNDS_CHECK, which was the whole point
of that macro.

* In the other functions I'd like to encourage you to re-use more of the
existing functions, i.e. have copy() simply call operator=() or vice versa.

> Please take a close look at the documentation. I'm not that much
> experienced with doxygen syntax... Most parts I have copied from gmd.h.
> I hope this is okay. :)

Sure. Looks good. I'll have one last round of documentation proofreading
 during the next hour, but then you have those files (in CVS) back to
your work without more interference from me.

> The class in sybfd.h should be clear. The function
> LaSymmBandMatFactorize is mostly original. The serious error was
> 
> integer = A.size(0)
> 
> what was incorrect, because it returns the wrong dimension.
> 
> The first LaLinearSolve-function is original too. I only set the info
> flags...

Ok. Looks good on a first glance. I accepted your file unchanged, but
mostly because the original one was complete nonsense anyway :-) . If
possible, please think about splitting the code to a .cc file so that
only the useful functions stay inline and the others are defined in the
.cc file. Also, please think about adding a test program (in
matrix/test/blablabla.cc) so that the correct functioning can be tested
automatically.

> The second LaLinearSolve-function is because often people want to solve
> a matrix and have no need to factorize them before. So this function
> will do it.

I would prefer not to have such convenience functions, because
eventually this might drive people to not think about the internally
used factorization. Once people know that the solving is done in two
steps, they might keep in mind that the left hand side in many
applications could be reused several times, hence they would require
only half the calculations. However, the decision about which functions
to add and which not is obviously in part a matter of taste, so in this
case it's probably fine. In that LaLinearSolve() case you even made this
quite clear by simply calling these other functions.

Actually I'm not so sure whether the function LaSymmBandMatFactorize()
is useful. If that functionality better lives inside some
LaSymmBandFactDouble class methods then I'd encourage you to remove the
function and move the code to suitable class methods.

> The LaLinearSolveIP-function is because I think often the arguments can
> be overwritten because they are not needed any longer. So it would be
> rather useless to allocate memory for a new factorization. I hope you agree.

An additional "IP"-function is almost always a *good* thing. You might
even think about implementing the non-IP version of that function in
terms of the IP-version... but I think I've already said this.

> Several other things:
> 
> 1. You have written in gmd.h
> 
>        * Optional runtime bounds checking (0<=i<=m, 0<=j<=n) is set
>        * by the compile time macro LA_BOUNDS_CHECK.
> 
> This is wrong, it should be (0<=i<m, 0<=j<n), m and n are already out of
> bounds.

You are correct. The comment is wrong.

> 2. What is the correct meaning of A.size(int d) ? I think the code
> 
>   for (int i=0; i<A.size(0); i++)
>     for (int j=0; j<A.size(1); j++)
>       A(i,j)=1.0;
> 
> should never fail. Do you agree? 

No, I think size() should give the dimensions in terms of the
"mathematical" viewpoint, not in terms of the internal storage
implementation. So I think this code in fact does fail as soon as the
internal storage is different from the "mathematical" matrix dimension
in question, which is probably true for all matrix classes except the
"General" (dense) ones. At least that's what I *thought* until I started
to look at LaSymmBandMatDouble::size() --

> If so, the size-function in
> LaSymmBandMatDouble is still wrong, because it does not return size(0)=N
> and size(1)=N but size(0)=2*P+1 and size(1)=N. I don't know if it's part
> of the programmer (of the library) to set this correct so that size(0)
> returns N or if the user of the library should pay attention to this.
> What do you think?

The size() function in LaSymmBandMatDouble might very well be wrong. If
you're redesigning this class, the decision is pretty much up to you.
Only make sure you clearly explain the meaning of size(), especially if
it is different from my described meaning above which would hold for the
General/dense matrices.

Regards,

Christian


[1]
http://cstimming.de/index.php?option=com_content&task=view&id=28&Itemid=41

[2] http://sourceforge.net/cvs/?group_id=99696
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQCVAwUBRV2RwmXAi+BfhivFAQJ+DwP8CcQVRXtK2MAeQwRKeMJzArv9ctJcHsSU
KpRCrkLVTCDIBjYc9ycIUO6d5YxVNvzUK0iUCHT4b74UtczkxiQIH57kPhDhXG1f
zMW0r3UCAhlvt88QaHqw9CjzhAtZtP4rrMct/sjMrRife46B1o95ETZGpqF8pMtB
p7ma2djAVMQ=
=XuiU
-----END PGP SIGNATURE-----