Update to StFcsWaveformFitMaker to use new algorithm #401

dkapukchyan · 2022-09-16T23:53:49Z

This is an update to the StFcsWaveformFitMaker algorithm to improve the peak finding method and energy determination from the raw signal data.

plexoos · 2022-09-19T17:38:45Z

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.cxx

    }
    if(mMeasureTime){
-      mTime=new TH1F("FitTime","FitTime; msec",200,0,6);
+      mTime=new TH1F("FitTime","FitTime;msec",1000,0,1000);


I am using this line to comment on a common problem which this PR seems to introduce in quite a few other places.
The mTime histogram can be allocated multiple times during a job when the framework calls InitRun(), however, the corresponding object is deleted only once in the destructor, hence causing a memory leak. Depending on the logic you are trying to implement here, you may need to allocate histograms only once in the Init() method. It may be also easier to rely on the automatic object lifetime than trying to manage the memory manually like you do.

I thought InitRun was only called once during a job, which is why I put the histogram creations there. I agree that I only need these histograms created once per job. Where should I move the histogram allocations so that they only get created once? Hopefully I can move these before this pull request is made so that I don't need to make a new one.

Move to Init(). In principle, InitRun can be called multiple times per job if you are reading file(s) from multiple runs.

If it needs to be done only once, is there a reason not to initialize the histograms in the constructor? Also, if there is no good reason to allocate them with the new operator, you can avoid the many delete statements in the destructor.

NB: During StMaker::Init(), any newly instantiated histograms are added to the maker's "hist" subdirectory (in TDataset terms) which allows them to be found using the TDataset methods and enables inclusion in the BFC's hist.root output files.

// Add the Maker histograms in the Maker histograms list
// and remove it from the ROOT system directory
gROOT->cd();
TIter nextHist(gDirectory->GetList());
Int_t ready = !objLast;
while((objHist=nextHist())) {// loop over gDirectory
if (!ready && objHist!=objLast) continue;
ready = 1999;
if (objHist==objLast) continue;
if (!objHist->InheritsFrom("TH1")) continue;

// Move the histogram from the ROOT list into the "maker's" list
((TH1*)objHist)->SetDirectory(0);
maker->AddHist((TH1*)objHist);
}

Moving them to the constructor means that these histograms will always be created, whereas now they are only created when a certain test level is set using setTest(). I thought this would be best since in the production these histograms don't need to be created as they are mostly used to quantify how well the algorithm is working. If I move them to the constructor then I would have to add an argument for the test level and file name where those histograms will be saved. I think doing that will only make this class more complicated so I left them in Init(). That way whoever calls this class can turn testing on or off before it hits the chain.

plexoos

StFcsWaveformFitMaker is already included in the fcs chain option. According to this page https://www.star.bnl.gov/devcgi/dbProdOptionRetrv.pl it was used once in
pp2022 StiCA BEmcChkStat epdhit -hitfilt for "Preliminary chain for run 2022 fastoffline production". From this I conclude that changing the algorithm should not have significant consequences

StRoot/StFcsWaveformFitMaker/PeakWindow.cxx

StRoot/StFcsWaveformFitMaker/PeakAnaVirtualPainter.h

StRoot/StFcsWaveformFitMaker/PeakAnaPainter.h

StRoot/StFcsDbMaker/StFcsDbMaker.h

plexoos · 2022-09-27T16:23:08Z

StRoot/StFcsWaveformFitMaker/PeakAna.cxx

+  {
+    graph->SetPoint(ipeak, Ana.GetPeak(ipeak).mPeakX, Ana.GetPeak(ipeak).MidPoint() );
+  }
+  PeakAna* NewAna = new PeakAna(Ana,graph);


This can cause a memory leak. Why not return the object? EDIT: I mean "return by value"

I am not sure whether you are referring to graph or NewAna. I don't think graph will give a memory leak since the next line calls NewAna->ForceInternal(). This means that the newly created graph object is "owned" by NewAna and it will delete that newly created graph when it is destroyed or SetData() is called. There is no way to change mInternalSignal to false after that so NewAna will never think that it doesn't own graph. If you are referring to NewAna then I agree it will if someone forgets to delete it. I don't want to return the graph object itself because if someone does PeakAna* newana = new PeakAna(OldAna,ConvertPeaksToAna(OldAna)) then the way this constructor works it will think the graph object is external to it and it won't delete it and that will be a memory leak. Which means that the caller will have to remember to always call newana->ForceInternal() in this situation. Here I called it automatically to prevent such a situation so that's why I don't return the graph object.

Actually, I meant you could return the NewAna object by value rather than by pointer, not the graph object. I am a bit lost on whether PeakAna owns the graph object or not and under what conditions... Hopefully, this complex (for me) logic is correct and there are no memory leaks.

I thought about returning by value but my understanding is that when you return by value it returns a copy of the object not the object created in the function itself. This is not something I want for a large object like PeakAna. Also, if you try to get around this by returning a reference instead then the returned reference is to the object created in the function which has now been invalidated and hence does not exist. This is why I return the pointer to the new object. Is my understanding incorrect and is there a safe way to do this without copying the object?

It is safe to assume that your compiler implements the return value optimization. Moreover, copy elision is guaranteed when an object is returned directly (as of C++17)

Is it going to be addressed?

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.h

plexoos · 2022-09-27T16:26:47Z

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.h

+    virtual int Init();
+    virtual int InitRun(int runNumber);
+    virtual int Make();
+    virtual int Finish(); 
+    virtual void Clear(Option_t* option = "");


What you should really use here is the override keyword rather than virtual

I tried using override and compiling with cons and it complains that symbol override is not defined. Can you provide a working example?

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.h

starsdong · 2022-10-21T17:33:55Z

Akio, Daniel, we should try to conclude on this PR. As codeowners, could you please review and approve this PR?
Dmitri, in the meantime, the PR seems to be out-of-sync with main branch. Could you update the branch?

dkapukchyan · 2022-10-31T15:28:36Z

Xilin looked at invariant mass plot and finds no difference in the pi0 peak position and shape using the StFcsWaveformFitMaker in this commit and current StFcsWaveformFitMaker.

akioogawa

This is now well tested and ready to go.

plexoos · 2022-11-01T15:17:03Z

StRoot/StFcsWaveformFitMaker/StFcsPulseAna.cxx

+  if(mH1_Baseline!=0)     { delete mH1_Baseline; }
+  if(mF1_SignalFit!=0)    { delete mF1_SignalFit; }
+  if(mF1_BaselineFit!=0)  { delete mF1_BaselineFit; }
+


Suggested change

if(mH1_Baseline!=0) { delete mH1_Baseline; }

if(mF1_SignalFit!=0) { delete mF1_SignalFit; }

if(mF1_BaselineFit!=0) { delete mF1_BaselineFit; }

delete mH1_Baseline;

delete mF1_SignalFit;

delete mF1_BaselineFit;

plexoos · 2022-11-01T15:24:45Z

StRoot/StFcsWaveformFitMaker/StFcsPulseAna.cxx

+      //if( peak>0 && radc<=last) peak ^= 1<<rightshift ;
+      //std::cout << "    - SumDep0Mod::Case2|peak:"<<peak << "|radc:"<<radc<<"|last:"<<last << std::endl;
+      //if( peak!=0 || radc<last) peak = -1 ;


Can you please clean up the code and remove the commented code in all modified files? These distract focus from the important lines

I removed what I could. Please let me know if there is more that I missed. Long comments describing what the algorithms do and how they work I left in since I don't know where else to put them.

plexoos · 2022-11-07T20:08:47Z

StRoot/StFcsWaveformFitMaker/PeakAna.cxx

+  {
+    graph->SetPoint(ipeak, Ana.GetPeak(ipeak).mPeakX, Ana.GetPeak(ipeak).MidPoint() );
+  }
+  PeakAna* NewAna = new PeakAna(Ana,graph);


Is it going to be addressed?

plexoos · 2022-11-07T20:58:22Z

StRoot/StFcsWaveformFitMaker/PeakAna.cxx

+  std::cout << "|P::Graph:"<<mG_Data;
+  std::cout << "|RangeX:["<<mXRangeMin<<","<<mXRangeMax<<"]";
+  std::cout << "|RangeY:["<<mYRangeMin<<","<<mYRangeMax<<"]";


Is it going to be addressed?

plexoos · 2022-11-07T21:00:52Z

StRoot/StFcsWaveformFitMaker/PeakAna.cxx

+
+PeakAna* PeakAna::MeanFilter( Int_t sizeavgs, bool copy )
+{
+  if( this->GetData()==0 ){return this;}


You use this-> a lot for class members. In most of our code it is usually omitted. I suggest you do the same.

Removed what I found. I only left the ones relating to virtual methods.

StRoot/StFcsWaveformFitMaker/StFcsPulseAna.h

plexoos · 2022-11-07T21:07:40Z

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.cxx

+    if( mH1_NPeaksAkio!=0 ){ mH1_NPeaksAkio->Write(); }
+    if( mH1_NPeaksFilteredAkio!=0 ){ mH1_NPeaksFilteredAkio->Write(); }
+    if( mH1_PeakTiming!=0 ){mH1_PeakTiming->Write();}
+
+    if( mH2_NPeakvsPeakXdiff!=0 )      {mH2_NPeakvsPeakXdiff->Write();}
+    if( mH2_NPeakvsPeakYratio!=0)      {mH2_NPeakvsPeakYratio->Write();}
+    if( mH1_VOverlap!=0         )      {mH1_VOverlap->Write();}


Have you tried to make a single call to rootFile->Write(); instead of doing this for individual objects?

I have tried it before for other classes and it doesn't always work. It probably has to do with the order in which you create TFile and histograms so I explicitly write them in now. If it is really a big problem I can try to see if rootFile->Write() will work in this instance.

Yes, please try again. ROOT has this notion of a current directory so, when you open a new file it switches to that current directory and it becomes a parent for all TObject-s created after that. The common trick with ROOT is to save the current directory and restore it later when you are done creating your histograms. Since you do it in a Maker this can interfere with other opened files, although I recall you have a switch which disables histogram filling by default. Since no tests were provided for all of this functionality we can only hope that it all works as intended.

I tried with just rootFile->Write() and it didn't work. I understand the issue but I don't know what else to do to avoid this. You are correct that if no test level is called then no output needs to be saved. The test levels are for single file tests and should never be called in production. They are intended to help understand and improve the algorithm rather than for generating generic QA histograms for StFcsWaveformFitMaker.

plexoos · 2022-11-09T19:16:14Z

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.cxx

+    if( mH1_NPeaksAkio!=0 ){ mH1_NPeaksAkio->Write(); }
+    if( mH1_NPeaksFilteredAkio!=0 ){ mH1_NPeaksFilteredAkio->Write(); }
+    if( mH1_PeakTiming!=0 ){mH1_PeakTiming->Write();}
+
+    if( mH2_NPeakvsPeakXdiff!=0 )      {mH2_NPeakvsPeakXdiff->Write();}
+    if( mH2_NPeakvsPeakYratio!=0)      {mH2_NPeakvsPeakYratio->Write();}
+    if( mH1_VOverlap!=0         )      {mH1_VOverlap->Write();}


Yes, please try again. ROOT has this notion of a current directory so, when you open a new file it switches to that current directory and it becomes a parent for all TObject-s created after that. The common trick with ROOT is to save the current directory and restore it later when you are done creating your histograms. Since you do it in a Maker this can interfere with other opened files, although I recall you have a switch which disables histogram filling by default. Since no tests were provided for all of this functionality we can only hope that it all works as intended.

plexoos · 2022-11-09T20:17:32Z

StRoot/StFcsWaveformFitMaker/PeakAna.cxx

+  std::cout << "|P::Graph:"<<mG_Data;
+  std::cout << "|RangeX:["<<mXRangeMin<<","<<mXRangeMax<<"]";
+  std::cout << "|RangeY:["<<mYRangeMin<<","<<mYRangeMax<<"]";


I am not aware of any exclusion to the rule requiring the use of LOG_ stream objects. The advantage is that we can turn off or limit logging to a predefined level. I hope in production only errors and maybe warnings are logged to avoid unnecessary writes to, for example, LOG_DEBUG and LOG_INFO streams

plexoos · 2022-11-09T20:27:31Z

StRoot/StFcsWaveformFitMaker/StFcsWaveformFitMaker.cxx

+  if( mH1_PeakTimingGaus!=0 )     {delete mH1_PeakTimingGaus;}
+  if( mH1_PeakTimingPuls!=0 )     {delete mH1_PeakTimingPuls;}
+  if( mH2_PeakTimingCompare!=0 )  {delete mH2_PeakTimingCompare;}


I thought we already discussed this: there is no need to check for the pointer to be non null before deleting the object. Isn't nicer to have less code than more code?

Suggested change

if( mH1_PeakTimingGaus!=0 ) {delete mH1_PeakTimingGaus;}

if( mH1_PeakTimingPuls!=0 ) {delete mH1_PeakTimingPuls;}

if( mH2_PeakTimingCompare!=0 ) {delete mH2_PeakTimingCompare;}

delete mH1_PeakTimingGaus;

delete mH1_PeakTimingPuls;

delete mH2_PeakTimingCompare;

Done. Sorry I had forgotten about this file :)

dkapukchyan added 2 commits September 16, 2022 19:17

Updates to StFcsWaveformFitMaker

cc830c3

Small fixes to updates for StFcsWaveformFitMaker

61ccc88

dkapukchyan requested review from akioogawa and jdbrice as code owners September 16, 2022 23:53

fix to match sum8 to fitted sum

8303312

plexoos reviewed Sep 19, 2022

View reviewed changes

Move histogram creation to Init() in StFcsWaveformFitMaker

0129ca6

plexoos reviewed Sep 27, 2022

View reviewed changes

dkapukchyan added 5 commits September 28, 2022 17:08

Some fixes based on comments from github

c80e2fe

Removed PeakAnaVirtualPainter

0ba342a

Added Doxygen style comments and some cleanup

6fbe46a

Added test level 7 for StFcsWaveformFitMaker and minor fixes

dd661ed

Merge remote-tracking branch 'upstream/main' into main

add45f2

akioogawa approved these changes Nov 1, 2022

View reviewed changes

plexoos reviewed Nov 7, 2022

View reviewed changes

Mostly cleanup and some fixes for compiling

a70e2a3

plexoos reviewed Nov 9, 2022

View reviewed changes

More cleanup and fixes

ae6a392

jdbrice approved these changes Nov 17, 2022

View reviewed changes

jdbrice merged commit 347894e into star-bnl:main Nov 17, 2022

Update to StFcsWaveformFitMaker to use new algorithm #401

Update to StFcsWaveformFitMaker to use new algorithm #401

Conversation

dkapukchyan commented Sep 16, 2022

plexoos Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

dkapukchyan Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plexoos left a comment

Choose a reason for hiding this comment

plexoos Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starsdong commented Oct 21, 2022

dkapukchyan commented Oct 31, 2022

akioogawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plexoos Sep 19, 2022 •

edited

Loading

dkapukchyan Sep 20, 2022 •

edited

Loading

plexoos Sep 27, 2022 •

edited

Loading