Bug 129168

Summary: KstPSDGenerator and KstPSD contain duplicate code.
Product: [Applications] kst Reporter: Duncan Hanson <duncan.hanson>
Component: generalAssignee: Duncan Hanson <duncan.hanson>
Status: RESOLVED FIXED    
Severity: normal    
Priority: NOR    
Version: 1.x   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description Duncan Hanson 2006-06-15 01:36:28 UTC
Version:            (using KDE KDE 3.5.0)
Installed from:    Compiled From Sources
OS:                Linux

Hi all-

I'd like to remove the KstPSDGenerator and have KstCSD use KstPSD instead. There's almost complete code duplication right now.

To do this, I'll probably move from QValueVectors to something like KstVector for holding the CSD's temporary power spectrum. There may be a slight performance hit, but I think It should be manageable. 

Does this sound good?

Duncan.
Comment 1 George Staikos 2006-06-15 02:33:38 UTC
   KstCSD should not use KstPSD.  It breaks the object model.  Furthermore 
please do not introduce new performance penalties.  We spend a huge amount of 
time profiling Kst and we don't want to lose that effort.  The use of 
QValueVector is already a performance loss compared with what we had before.  
I'd rather see KstPSDGenerator fixed and have both KstPSD and KstCSD use it.
Comment 2 Duncan Hanson 2006-06-19 19:48:03 UTC
Hi George,

Ok then- how about I make a utility class called KstPSDCalculator which will be able to calculate a PSD for an array of doubles, and adapt both KstPSD and KstCSD to use it. Then we can remove KstPSDGenerator. There should be a performance increase in this case, because we'll also be getting rid of the QValueVectors.

In the meantime, can you please explain the 'object model' which is broken? To my ear, a CSD is nothing but a collection of PSDs, and so it makes perfect sense to have a CSD using them. 
Comment 3 George Staikos 2006-06-19 22:48:55 UTC
> Ok then- how about I make a utility class called KstPSDCalculator which
> will be able to calculate a PSD for an array of doubles, and adapt both
> KstPSD and KstCSD to use it. Then we can remove KstPSDGenerator. There
> should be a performance increase in this case, because we'll also be
> getting rid of the QValueVectors.


   Ok sounds fine to me.

> In the meantime, can you please explain the 'object model' which is broken?
> To my ear, a CSD is nothing but a collection of PSDs, and so it makes
> perfect sense to have a CSD using them.


  A 'PSD' and 'KstPSD' are distinctly different things.  KstPSD is a 
dataobject implementation that represents specifically one PSD.  The data 
objects are designed to be chained via inputs and outputs of primitives only.  
If something else needs the PSD algorithms, it should do so via a shared PSD 
implementation, not the KstPSD object itself.
Comment 4 Duncan Hanson 2006-06-22 01:46:37 UTC
SVN commit 553760 by dhanson:

CCBUG:129168 copy KstPSDGenerator code into PSDCalculator utility class. comment some of the numerical factors so we know where they come from. next step will be converting KstCSD to use this class, and then KstPSD.

 A             psdcalculator.cpp   [License: GPL (v2+)]
 A             psdcalculator.h   [License: GPL (v2+)]
Comment 5 Duncan Hanson 2006-06-22 19:22:29 UTC
SVN commit 553967 by dhanson:

CCBUG:129168 move KstCSD over to PSDCalculator.

 M  +80 -96    kstcsd.cpp  
 M  +11 -6     kstcsd.h  
Comment 6 Duncan Hanson 2006-06-22 19:23:52 UTC
SVN commit 553968 by dhanson:

CCBUG:129168 add PSDCalculator to Makefile.am so that KstCSD will compile.

 M  +1 -0      Makefile.am  


--- trunk/extragear/graphics/kst/src/libkstmath/Makefile.am #553967:553968
@@ -37,6 +37,7 @@
 	dialoglauncher.cpp \
 	eparse-eh.cpp \
 	eparse.c \
+	psdcalculator.cpp \
 	escan.c
 
 libkstmath_la_LDFLAGS = -version-info @KST_LIBKST_VERSION@ $(all_libraries) -no-undefined
Comment 7 Duncan Hanson 2006-06-22 20:19:34 UTC
SVN commit 553979 by dhanson:

CCBUG:129168 let PSDCalculator interpolate holes.

 M  +20 -4     psdcalculator.cpp  


--- trunk/extragear/graphics/kst/src/libkstmath/psdcalculator.cpp #553978:553979
@@ -26,6 +26,7 @@
 
 #include "kstdebug.h"
 #include "psdcalculator.h"
+#include "kstvector.h"
 
 extern "C" void rdft(int n, int isgn, double *a);
 
@@ -153,8 +154,6 @@
   bool apodize, ApodizeFunction apodizeFxn, double gaussianSigma,
   PSDType outputType, double inputSamplingFreq) {
 
-  Q_UNUSED(interpolateHoles) // right now, interpolation for holes will be done to the input data before passing it in to calculatePowerSpectrum. we can get a speed increase if we change this in the future...
-
   if (outputLen != calculateOutputVectorLength(inputLen, average, averageLen)) {
     KstDebug::self()->log(i18n("in PSDCalculator::calculatePowerSpectrum: received output array with wrong length."), KstDebug::Error);
     return -1;
@@ -212,10 +211,23 @@
     }
 
     // apply the PSD options (removeMean, apodize, etc.)
-    if (removeMean && apodize) {
+    // separate cases for speed- although this shouldn't really matter- the rdft should be the most time consuming step by far for any large data set.
+    if (removeMean && apodize && interpolateHoles) {
       for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
+        _a[i_samp] = (kstInterpolateNoHoles(input, inputLen, i_samp + ioffset, inputLen) - mean)*_w[i_samp];
+      }
+    } else if (removeMean && apodize) {
+      for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
         _a[i_samp] = (input[i_samp + ioffset] - mean)*_w[i_samp];
       }
+    } else if (removeMean && interpolateHoles) {
+      for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
+        _a[i_samp] = kstInterpolateNoHoles(input, inputLen, i_samp + ioffset, inputLen) - mean;
+      }
+    } else if (apodize && interpolateHoles) {
+      for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
+        _a[i_samp] = kstInterpolateNoHoles(input, inputLen, i_samp + ioffset, inputLen)*_w[i_samp];
+      }
     } else if (removeMean) {
       for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
         _a[i_samp] = input[i_samp + ioffset] - mean;
@@ -224,6 +236,10 @@
       for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
         _a[i_samp] = input[i_samp + ioffset]*_w[i_samp];
       }
+    } else if (interpolateHoles) {
+      for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
+        _a[i_samp] = kstInterpolateNoHoles(input, inputLen, i_samp + ioffset, inputLen);
+      }
     } else {
       for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
         _a[i_samp] = input[i_samp + ioffset];
@@ -239,7 +255,7 @@
     }
   }
 
-  double frequencyStep = inputSamplingFreq/_awLen;
+  double frequencyStep = .5*(double)inputSamplingFreq/(double)(outputLen-1);
 
   //normalization factors which were left out earlier for speed. 
   //    - 2.0 for the negative frequencies which were neglected by the rdft //FIXME: double check.
Comment 8 Duncan Hanson 2006-06-22 23:46:06 UTC
SVN commit 554017 by dhanson:

CCBUG:129168 backwards compatibility. not storing _averageLen is kind of strange though...

 M  +3 -2      kstcsd.cpp  


--- trunk/extragear/graphics/kst/src/libkstmath/kstcsd.cpp #554016:554017
@@ -181,6 +181,7 @@
 
   double *tempOutput, *input;
   int tempOutputLen = PSDCalculator::calculateOutputVectorLength(_windowSize, _average, _averageLength);
+  _PSDLen = tempOutputLen;
   tempOutput = new double[tempOutputLen];
 
   input = inVector->value();
@@ -227,7 +228,7 @@
   ts << l2 << "<vectag>" << QStyleSheet::escape(_inputVectors[INVECTOR]->tagName()) << "</vectag>" << endl;
   ts << l2 << "<sampRate>"  << _frequency << "</sampRate>" << endl;
   ts << l2 << "<average>" << _average << "</average>" << endl;
-  ts << l2 << "<fftLen>" << _averageLength << "</fftLen>" << endl;
+  ts << l2 << "<fftLen>" << int(ceil(log(double(_PSDLen*2)) / log(2.0))) << "</fftLen>" << endl;
   ts << l2 << "<removeMean>" << _removeMean << "</removeMean>" << endl;
   ts << l2 << "<apodize>" << _apodize << "</apodize>" << endl;
   ts << l2 << "<apodizefxn>" << _apodizeFxn << "</apodizefxn>" << endl;
@@ -345,7 +346,7 @@
 }
 
 int KstCSD::length() const { 
-  return _averageLength;  
+  return _averageLength;
 }
 
 void KstCSD::setLength(int in_length) {
Comment 9 Duncan Hanson 2006-06-22 23:47:11 UTC
SVN commit 554018 by dhanson:

CCBUG:129168 backwards compatibility. not storing _averageLen is kind of strange though...

 M  +1 -0      kstcsd.h  


--- trunk/extragear/graphics/kst/src/libkstmath/kstcsd.h #554017:554018
@@ -103,6 +103,7 @@
     double _gaussianSigma;
     int _windowSize;
     int _averageLength;
+    int _PSDLen;
     QString _vectorUnits;
     QString _rateUnits;
 
Comment 10 Duncan Hanson 2006-06-22 23:56:37 UTC
SVN commit 554020 by dhanson:

CCBUG:129168 have KstPSD use PSDCalculator. use enumed apodization functions.

 M  +1 -1      kst/main.cpp  
 M  +1 -1      libkstapp/datawizard.ui.h  
 M  +3 -1      libkstapp/kstiface_impl.cpp  
 M  +2 -2      libkstapp/kstpsddialog_i.cpp  
 M  +51 -216   libkstmath/kstpsd.cpp  
 M  +13 -15    libkstmath/kstpsd.h  
Comment 11 Duncan Hanson 2006-07-12 03:02:08 UTC
hi george,

re: my earlier email. i can now confirm the problem. the power spectrum
produced by psdcalculator is a bit too high at the low frequency end.
good eye.

i've identified the issue- i divided by an incorrect variable in the
'new' psdcalculator. i'll commit a fix.

duncan.

(the blank plots were because gyrodata.dat wasn't being located- has the
way in which kst attaches data files changed recently?)

On Tue, 2006-07-11 at 19:02 -0400, George Staikos wrote:
> Seems to me that PSDs are not working right anymore.  demo.kst has quite an 
> odd look to it.  Can anyone else confirm?
> 

Comment 12 Duncan Hanson 2006-07-12 03:05:01 UTC
SVN commit 561384 by dhanson:

CCBUG:129168 fix incorrect division in psdcalculator

 M  +1 -1      psdcalculator.cpp  


--- trunk/extragear/graphics/kst/src/libkstmath/psdcalculator.cpp #561383:561384
@@ -199,7 +199,7 @@
       for (i_samp = 0; i_samp < currentCopyLen; i_samp++) {
         mean += input[i_samp + ioffset];
       }
-      mean /= (double)_awLen; // _awLen != 0 b/c outputLen != 0.
+      mean /= (double)currentCopyLen;
     }
 
     // apply the PSD options (removeMean, apodize, etc.)
Comment 13 Duncan Hanson 2007-02-07 00:07:16 UTC
*** Bug has been marked as fixed ***.