Re: [Libreoffice] Help concerning code style and documentation

2010-11-04 Thread Kohei Yoshida
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

2010-11-03 Thread Kohei Yoshida
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

2010-11-01 Thread Thorsten Behrens
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

2010-11-01 Thread Kohei Yoshida
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

2010-10-28 Thread Michael Meeks
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)
-{