Bug fix for StFcsWaveformFitMaker and some minor additions #479
Conversation
…lso pedestal value is now stored in res[6] and pedestal error in res[7]
|
A lot more changes than a bug fix. But I believe you've already checked this produces good pi0 peaks and not loosing statistics. I approve. |
Yes, I forgot to mention that I got rid of some energy selects that are no longer relevant. I also added the pedestal computing functions and their corresponding energy selects for some methods; so now there are more energy selects so you can use my new methods with non-pedestal subtracted data. |
| } | ||
| ped = float(p)/(mPedMax-mPedMin+1.0); | ||
| pedstd = sqrt( ( sumsq-((double(p)*double(p))/(mPedMax-mPedMin+1.0)) )/(mPedMax-mPedMin) );//Variance/StdDev using naive algorithm from https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance | ||
| return ped; |
There was a problem hiding this comment.
Why return the same value if you modify the function argument?
There was a problem hiding this comment.
This has to do with the way the code was setup before. I didn't want to overhaul too many things and preserve the overall logic in Make. I would have to talk to @akioogawa before making such changes because it could break the way he has things set up and will require more time and testing to ensure that everything still works as intended.
| } | ||
| if(tb>=mPedMax) break; | ||
| } | ||
| ped = float(p)/(mPedMax-mPedMin+1.0); |
There was a problem hiding this comment.
You don't need to cast p to float. Remove, so it'd be easier to read the code. Why do you do the calculations in single precision rather than double?
There was a problem hiding this comment.
The reason is that ped and pedstd get stored in the array "res" which is of type float (found in the function Make). So I chose float to match the array type. If I change it to double the whole array would have to be changed to double so would any method that uses the "res" array, so I thought it would be easier to preserve the type. Would you prefer we change everything to double?
The bug fix is in the StFcsPulseAna::SetFitPars() and the (newly named) StFcsWaveformFitMaker::NPeaksPrePost() function. Any releases or data processed using the StFcsWaveformFitMaker after pull request 401 will have the wrong calorimter energy if the default option for energy select was used. I am not sure which past releases this constitutes. The problem was that the PulseFit2() algorithm is supposed to find and fit only to a select number of peaks (npeaks). The number of peaks it is supposed to fit is given by NPeaksPrePost(). Also, since the peaks are stored in a vector if the peak for the triggered crossing was now the second peak instead of the 4th this new index for the triggered crossing peak needed to be returned by NPeaksPrePost() so you pick up the correct parameter from the TF1. However, The function SetFitPars assumes that all peaks will need to be fit up to npeaks. This means if you have 5 peaks and only want to fit the 3rd and 4th peak; then npeaks=2 so SetFitPars would actually set the parameters for the 1st and 2nd peaks NOT the 3rd and 4th. This is the bug. The fix was to change SetFitPars to accept a starting index and ending index for which peaks to set the parameters for. Also, NPeaksPrePost() needed to be changed to return a vector of indexes for peaks that need to be fit. The trigfitidx now tells you the index in the vector of indexes where the triggered crossing was. This was done in this way because trigfitidx will match the index of the parameter space of the TF1 where to find the needed value. Most of the other code was added to find this bug. |
| printf("Pedestal=%6.2f/(%6.2f-%6.2f+1)=%6.2f +- %6.2f\n",float(p),float(mPedMax),float(mPedMin),ped,res[7]); | ||
| return gausFit(g, res, func, ped); | ||
| AnaPed( g, res[6], res[7] ); | ||
| printf("Pedestal (%6.2f-%6.2f+1)=%6.2f +- %6.2f\n",float(mPedMax),float(mPedMin),res[6],res[7]); |
There was a problem hiding this comment.
Another printf should be changed to LOG_INFO. It is kind of important if this maker is to be used in production
There was a problem hiding this comment.
Changed to LOG_DEBUG
This is a fix for a bug in StFcsWaveformFitMaker where the fit parameters where not being set correctly. The ability to recompute energy without fitting for the integral is added. Added some pedestal computing functions. Also, more doxygen style documentation.