Re: [Libreoffice] Help concerning code style and documentation
On Wed, 2010-11-03 at 09:39 -0400, Kohei Yoshida wrote: You know, that I work on it in my spare time, so response time might be long. You should tell me, whether the mathematics I used are clear from the code or you want me to write a description. Understood. We certainly want you to do it at your own pace. :-) As for documentation of the mathematics used behind the calculations, if you could write up some overview of it, it would certainly help us maintain it in the future. Speaking for myself, I'm not too familiar with the math involved there, so BTW, if there is a wikipedia page or similar that you can link to, just providing that URL would be sufficient. Kohei -- Kohei Yoshida, LibreOffice hacker, Calc kyosh...@novell.com ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Help concerning code style and documentation
Hi Regina, On Wed, 2010-11-03 at 10:35 +0100, Regina Henschel wrote: Hi Kohei, Kohei Yoshida schrieb: Hi Regina, On Sun, 2010-10-24 at 18:13 +0200, Regina Henschel wrote: Hi all, I'm currently working on LINEST and have attached a draft to issue http://www.openoffice.org/issues/show_bug.cgi?id=115189 There is no mathematical problem, but I'm uncertain about coding style. The algorithms work on matrices. They have a lot of parts which are nearly identical but the matrices are transposed. How to handle that? If one algorithm traverses matrix row-wise, and another column-wise, then I would just leave them as-is. There isn't much we could do to reduce code side in that situation, and I don't think it would be worth the effort. If we really wanted, we could try templates as Thorsten suggested, but we can try that after your initial code gets integrated with some test cases to verify calculations. I have attached a true patch and some test cases to http://www.openoffice.org/issues/show_bug.cgi?id=115189 Excellent! Thanks. I'll take a look at that. Do you have already any other test cases around? No, unfortunately not. We will have to work on beefing up our test cases, for sure. FYI, I'm planning to integrate a major refactoring of the guts of ScMatrix, to push most of its implementation out to an external library and to allow different base values (numerical-0 vs empty elements). It should not affect ScInterpreter code as the change should be transparent to the users of ScMatrix, but I thought I would mention. And I would probably be the one to maintain your code, unless you want to maintain it yourself. ;-) You know, that I work on it in my spare time, so response time might be long. You should tell me, whether the mathematics I used are clear from the code or you want me to write a description. Understood. We certainly want you to do it at your own pace. :-) As for documentation of the mathematics used behind the calculations, if you could write up some overview of it, it would certainly help us maintain it in the future. Speaking for myself, I'm not too familiar with the math involved there, so Thanks a lot! Kohei -- Kohei Yoshida, LibreOffice hacker, Calc kyosh...@novell.com ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Help concerning code style and documentation
Regina Henschel wrote:
There is no mathematical problem, but I'm uncertain about coding
style. The algorithms work on matrices. They have a lot of parts
which are nearly identical but the matrices are transposed. How to
handle that?
Hi Regina,
I'd suggest using templates to achieve that - this way, you'll get
the same runtime performance, but avoid all the (semantically)
duplicated code.
There are multiple ways to tackle that, to avoid changing your code
too much, I'd suggest passing a permutating type as template
parameter to each of the methods in need of variation, e.g.:
template class Selector
bool calculateQRdecomposition(
ScMatrixRef pMatA,
::std::vector double pVecR,
SCSIZE nK,
SCSIZE nN,
Selector const rSelector )
{
for (SCSIZE major = 0; major rSelector.getMajor(nK,nN); major+)
...
for (SCSIZE minor = major; minor rSelector.getMinor(nK,nN); minor++)
pMatA-PutDouble( pMatA-GetDouble(
rSelector.getCol(major,minor),
rSelector.getRow(major,minor))/fScale,
rSelector.getCol(major,minor),
rSelector.getRow(major,minor));
etc - the Selector class then has simple inline
double blah(a,b) { return a; } permutation methods - if you like, we
could have a look at that during the HackFest next weekend.
Otherwise - lovely code!
Cheers,
-- Thorsten
pgpa52g8Hkh2U.pgp
Description: PGP signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Help concerning code style and documentation
Hi Regina,
On Sun, 2010-10-24 at 18:13 +0200, Regina Henschel wrote:
Hi all,
I'm currently working on LINEST and have attached a draft to issue
http://www.openoffice.org/issues/show_bug.cgi?id=115189
There is no mathematical problem, but I'm uncertain about coding style.
The algorithms work on matrices. They have a lot of parts which are
nearly identical but the matrices are transposed. How to handle that?
If one algorithm traverses matrix row-wise, and another column-wise,
then I would just leave them as-is. There isn't much we could do to
reduce code side in that situation, and I don't think it would be worth
the effort.
If we really wanted, we could try templates as Thorsten suggested, but
we can try that after your initial code gets integrated with some test
cases to verify calculations.
And I do not know how much comments are needed for those, who have to
maintain the code later, and if a separate documentation is needed.
As Michael said, your level of comments look just fine to me. :-) The
only thing I would change is to remove commenting of closing braces.
For instance,
if (nCase == 2)
{
...
} // end of nCase == 2
type of comment is a bit too much since most modern editors should
detect the begin end brace pair. I would consider this to be
over-commenting. But other than that, your code looks just fine.
FYI, I'm planning to integrate a major refactoring of the guts of
ScMatrix, to push most of its implementation out to an external library
and to allow different base values (numerical-0 vs empty elements). It
should not affect ScInterpreter code as the change should be transparent
to the users of ScMatrix, but I thought I would mention.
And I would probably be the one to maintain your code, unless you want
to maintain it yourself. ;-)
Thanks a lot, and keep us informed of your progress.
Kohei
--
Kohei Yoshida, LibreOffice hacker, Calc
kyosh...@novell.com
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Help concerning code style and documentation
Hi Regina,
On Sun, 2010-10-24 at 18:13 +0200, Regina Henschel wrote:
I'm currently working on LINEST and have attached a draft to issue
Cool ! this is an awesome patch :-)
There is no mathematical problem, but I'm uncertain about coding style.
And your coding style looks beautiful, a great improvement over what
was there before.
The algorithms work on matrices. They have a lot of parts which are
nearly identical but the matrices are transposed. How to handle that?
Good question; and one to which I don't know the answer (sadly) -
whatever uses the least code, and yet performs well I suppose.
And I do not know how much comments are needed for those, who have to
maintain the code later, and if a separate documentation is needed.
Your level of commenting is great.
What are your opinions?
First - I'm sorry it took so long to get back to you; sad. Secondly
I've turned your changes into a patch (which I append). I split your
changes out into a new module - so that this (nice new) piece can be
licensed under the LGPLv3+/MPL combination - if you're happy with that.
Finally - you updated the CheckMatrix signature, but I didn't see the
header change for that; it'd be great to expand on the diff to include
those bits return it.
It'd be wonderful to have your changes included - though we have a
feature freeze in 2 days now ;-)
Many thanks looking forward to working with you [ do you hang out on
IRC ? it'd be great to introduce yourself there #libreoffice on
irc.freenode.net ].
Thanks !
Michael.
PS. do you think a self contained regression test in sc/qa/unit/ would
be good for this ? or does that belong better in a spreadsheet we can
load and compare results in ? [ it would be good to get some of them
into qa/unit so we can load / calculate and check the answers during
build I suspect ].
--
michael.me...@novell.com , Pseudo Engineer, itinerant idiot
diff --git a/sc/source/core/tool/interpr5.cxx b/sc/source/core/tool/interpr5.cxx
index ff72817..6fe6546 100644
--- a/sc/source/core/tool/interpr5.cxx
+++ b/sc/source/core/tool/interpr5.cxx
@@ -34,7 +34,6 @@
#include rtl/math.hxx
#include rtl/logfile.hxx
#include string.h
-#include math.h
#include stdio.h
#if OSL_DEBUG_LEVEL 1
@@ -2060,302 +2059,6 @@ void ScInterpreter::ScRGP()
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, sc, er, ScInterpreter::ScRGP );
CalulateRGPRKP(FALSE);
}
-bool ScInterpreter::CheckMatrix(BOOL _bLOG,BOOL _bTrendGrowth,BYTE nCase,SCSIZE nCX,SCSIZE nCY,SCSIZE nRX,SCSIZE nRY,SCSIZE M,SCSIZE N,ScMatrixRef pMatX,ScMatrixRef pMatY)
-{
-nCX = 0;
-nCY = 0;
-nRX = 0;
-nRY = 0;
-M = 0;
-N = 0;
-pMatY-GetDimensions(nCY, nRY);
-const SCSIZE nCountY = nCY * nRY;
-for ( SCSIZE i = 0; i nCountY; i++ )
-{
-if (!pMatY-IsValue(i))
-{
-PushIllegalArgument();
-return false;
-}
-}
-
-if ( _bLOG )
-{
-ScMatrixRef pNewY = pMatY-CloneIfConst();
-for (SCSIZE nElem = 0; nElem nCountY; nElem++)
-{
-const double fVal = pNewY-GetDouble(nElem);
-if (fVal = 0.0)
-{
-PushIllegalArgument();
-return false;
-}
-else
-pNewY-PutDouble(log(fVal), nElem);
-}
-pMatY = pNewY;
-}
-
-if (pMatX)
-{
-pMatX-GetDimensions(nCX, nRX);
-const SCSIZE nCountX = nCX * nRX;
-for ( SCSIZE i = 0; i nCountX; i++ )
-if (!pMatX-IsValue(i))
-{
-PushIllegalArgument();
-return false;
-}
-if (nCX == nCY nRX == nRY)
-nCase = 1; // einfache Regression
-else if (nCY != 1 nRY != 1)
-{
-PushIllegalArgument();
-return false;
-}
-else if (nCY == 1)
-{
-if (nRX != nRY)
-{
-PushIllegalArgument();
-return false;
-}
-else
-{
-nCase = 2; // zeilenweise
-N = nRY;
-M = nCX;
-}
-}
-else if (nCX != nCY)
-{
-PushIllegalArgument();
-return false;
-}
-else
-{
-nCase = 3; // spaltenweise
-N = nCY;
-M = nRX;
-}
-}
-else
-{
-pMatX = GetNewMat(nCY, nRY);
-if ( _bTrendGrowth )
-{
-nCX = nCY;
-nRX = nRY;
-}
-if (!pMatX)
-{
-PushIllegalArgument();
-return false;
-}
-for ( SCSIZE i = 1; i = nCountY; i++ )
-pMatX-PutDouble((double)i, i-1);
-nCase = 1;
-}
-return true;
-}
-void ScInterpreter::CalulateRGPRKP(BOOL _bRKP)
-{
