I see the code inside TRestSignalZeroSuppression has been updated. I believe that the code inside this process should be available to different processes.
Let me explain this …
In the previous version, the points over threshold were calculated/stored directly in the TRestRawSignal. This was done after calling the method GetIntegralWithThreshold (I am agree here that this was probably not the most intuitive thing, we should perhaps have a method that says InitPointsOverThreshold or similar).
Then, somehow, later, or in parallel, I could verify the points identified by TRestSignalZeroSuppression without using the process TRestSignalZeroSuppresion itself. I.e. plotting the raw signal with TRestRawSignalAnalysisProcess and TRestRawSignalViewerProcess.
In the present version, I wonder if the result of TRestSignalZeroSuppresion is the same as TRestRawSignalAnalysisProcess plus TRestRawSignalViewerProcess. Since now, TRestSignalZeroSuppression looks as if it is using its own independent code.
Then, I imagine the best solution would be to implement the code between lines 112 and 211 inside TRestRawSignal and TRestSignalZeroSuppresionProcess just calls that method and gets the points over threshold from TRestRawSignal to fill the output TRestSignal.
I modified this process because I need to implement a new parameter, pointsFlatThreshold, in order to better process the PandaX-III prototype TPC’s data. If we set this paratemer to 512, then the process behaves exactly the same as the older version.
I didn’t modify the code in TRestRawSignal because, after looking into it, I found it hard to quickly do the implementation, just by inserting some lines in the existing code. Compared with changing the whole logic of GetIntegralWithThreshold (), it seemed more straight forward to write an independent code to extract signals from raw signals. Then I wouldn’t change the result of RawSignalAnalysis(easier to compare with the old runs). In the meantime I can debug/test the new signal extraction alogrithm and try to find the best parameter to get the best track.
In future it is still possible to evolve the signal finding algorithm and parameters. But we won’t want to change the integration algorithm. Maybe we can add a new process that specifically uses the method TRestRawSignal::GetIntegralWithThreshold() to extract signals, while keeping TRestSignalZeroSuppression to use custom algorithm?
We need a method that determines which points are over threshold. This method should be in TRestRawSignal, and would receive any required parameters, fNPointsOverThreshold, pointsFlatThreshold, etc.
That way, TRestSignalZeroSuppressionProcess, TRestRawSignalAnalysisProcess, TRestRawSignalViewerProcess or other related processes would have access to the same routines to identify points over threshold, that would be filled in the temporary vector TRestRawSignal::fPointsOverThreshold.
GetIntegralWithThreshold would probably end up calling the new method that determines the points that are over threshold.
We could name that method something like → TRestRawSignal::InitializePointsOverThreshold. And it receives all parameters needed to determine which points are over threshold.
It could also be possible to use an option parameter to chose one or other code to identify the points over threshold.
What I was proposing in my first post was to migrate the actual code from TRestSignalZeroSuppresionProcess to TRestRawSignal::InitializePointsOverThreshold and TRestRawSignalEvent::InitializePointsOverThreshold.
Then TRestSignalZeroSuppression would just call this method TRestRawSignalEvent::InitializePointsOverThreshold and recover the points over threshold or other parameters by calling TRestRawSignal getter methods.
I think it is important that any process in REST uses the same logic to identify points over threshold, and that is embebed in the TRestRawSignal methods.
Just to provide an example, when I run TRestRawSignalAnalysisProcess+TRestRawSignalViewerProcess and TRestRawSignalZeroSuppression I get into different results.
In the following picture, on the left is the result of TRestSignalZeroSuppression, while on the right the green points identified over threshold by TRestRawSignalAnalysisProcess and TRestRawSignalViewerProcess.
I see your update of TRestSignalZeroSupressionProcess. I guess this introduced some difference of the physical result output.
There is a missing point I forgot to mention earlier in this post. The custom code of TRestSignalZeroSupressionProcess also did float digit correction for TRestRawSignalAnalysisProcess. In TRestRawSignalAnalysisProcess, we substract the baseline for its TRestRawSignalEvent. The point is that, “baseline” is a float digit, but the data points are short int. It will loose precision when doing the subtraction. So I correct it in TRestSignalZeroSupressionProcess.
I tested the output of commit 4958bbef and b3ad8189. Now this feature seems to be missing. The observable sAna_ThresholdIntegral should be the same with rA_CalibratedIntegral. The later one just intergrates all the points in TRestSignal’s data
I wanted to discuss this further in the forum, please, check the comments on TRestRawSignalAnalysisProcess. Now there is no baseline subtraction at the TRestRaw level.
GetData(n) will return directly the subtracted baseline result in float (double) precision. fBaseLine is calculated when calling CalculateBaseLine(from, to) that it is at the same time called by TRestRawSignalEvent::SetBaseLineRange( from, to).
It happened to me the same, the integral values of TRestRawSignalEvent::GetThresholdIntegral() inside TRestRawSignalAnalysisProcess and TRestSignalEvent::GetIntegral() after TRestSignalZeroSuppression were not the same.
In my case, the problem was that baseline definition was not the same, once I assured the same value on both processes I got the same result.
Sorry, I found the problem. It was because I am using different config file for the two versions. Now it seems OK. The results are all correct, thanks.
So now the baseline subtraction is done internally inside GetData()? I like this logic! Just several comments.
The method GetData() and GetSignalData() gives you different result, which is confusing. Maybe we rename GetData() to like GetSignalDataZs()?
The method AddOffset() should also add offset to the stored baseline value.
The method SubstractBaseline() is still converting double type baseline to short int type. Maybe we should remove that method?
Yes, I agree this is confusing. I would not mind to remove completely the method Short_t GetSignalData(). If we opt to change names we could rename Short_t GetSignalData to Short_t GetRawData. Any of both choices, removing or renaming, is fine with me. I don’t like GetSignalDataZs() because if fBaseline=0 there is no Zs.
I see, adding something like? if ( fBaseLine == 0 && fBaseLineSigma == 0 ) fBaseLine += offset;
I believe we should remove it to avoid unwanted precision problems to other users.
