Avoid code duplication for signal processing

[juanangp]

Some functions seems duplicated within TRestDetectorSignal and TRestRawSignal, for instance GetSignalSmoothed, GetBaseLine and GetBaseLineSigma.

I propose de following:

• Implement generic methods for signal processing that should be agnostic of the original signal event.
• Speed up data processing by extracting different parameters such as baseline, baselineSigma, threshold integral, risetime, maxPeak and so on using a single loop (funcion).
• The new class or namespace for signal processing should lie inside framework since I believe it should be generic for different libraries (rawlib detectorlib and connectorslib)

[jgalan]

Probably the methods on TRestDetectorSignal should be just removed. The methods you mention make sense only in the domain of raw-signal processing.

Actually, even if it is named signal, the TRestDetectorSignal has not, or in some scenarios will not, have the properties of a signal, and many times those algorithms will not make sense. The smoothing will require a fixed binning for example.

The reason why those methods are there is because of very old times (around 2016), when we didn't have a final definition of Short_t array that is today TRestRawSignalEvent.

In practice, TRestDetectorSignal is a transition between TRestDetectorHits and TRestRawSignal. It keeps physical times and amplitudes that will be used for event reconstruction.

[jgalan]

• Speed up data processing by extracting different parameters such as baseline, baselineSigma, threshold integral, risetime, maxPeak and so on using a single loop (funcion).

This is already done inside TRestRawSignalAnalysisProcess::ProcessEvent. If you think it is computationally expensive we could always create a new process with reduced calculations --> TRestRawSignalFastAnalysisProcess that extracts only the most basic observables.

Of course, this does not prevent us from having another method inside TRestRawSignal that gets all the pulse properties at once.

[jgalan]

• The new class or namespace for signal processing should lie inside framework since I believe it should be generic for different libraries (rawlib detectorlib and connectorslib)

However, those methods are clearly to be used with a TRetRawSignal.

[jgalan]

• Implement generic methods for signal processing that should be agnostic of the original signal event.

Agnostic? I imagine these methods will receive as argument a std::vector <Short_t>, right? That is exactly what it is a TRestRawSignal.

The existing methods are in some sense generic, but they operate on any Short_t vector.

[jgalan]

Perhaps some of the methods inside TRestDetectorSignal should be moved to TRestRawSignal. Several methods there date from long time ago when we didn't have yet a TRestRawSignal.

[juanangp]

In practice, TRestDetectorSignal is a transition between TRestDetectorHits and TRestRawSignal. It keeps physical times and amplitudes that will be used for event reconstruction.

I can see some other functions such as GetMaxPeakValue() that are used and are also duplicated.

Agnostic? I imagine these methods will receive as argument a std::vector <Short_t>, right? That is exactly what it is a TRestRawSignal.

The idea is to use a template std::vector <T>

However, those methods are clearly to be used with a TRetRawSignal.

Not necesary, one can think into perform the analysis of the smoothed signal, which is actually my problem. I would like to perform the analysis after signal smoothing and it is not straightforward rigth now. In fact, I would like to save the smoothed signal as a TRestDetectorSignal.

This is already done inside TRestRawSignalAnalysisProcess::ProcessEvent. If you think it is computationally expensive we could always create a new process with reduced calculations --> TRetRawSignalFastAnalysisProcess that extracts only the most basic observables.

We can keep the same funcions as before for signal processing, but I think we should add generic methods (functions) rather than processes.

[jgalan]

Not necesary, one can think into perform the analysis of the smoothed signal, which is actually my problem. I would like to perform the analysis after signal smoothing and it is not straightforward rigth now. In fact, I would like to save the smoothed signal as a TRestDetectorSignal.

We discussed this already with @lobis at some point. It is true this fact is missing. We discussed the idea about having another vector inside TRestRawSignal, for example std::vector <Double_t> fHDData; which is usually empty, but that we may fill with a given method. Once we identify this vector has been filled, the methods inside TRestRawSignal they can switch to operate on the high precision version.

We should try to find a solution that stays on the domain of rawlib.

[jgalan]

We can keep the same funcions as before for signal processing, but I think we should add generic methods (functions) rather than processes.

The generic methods are implemented inside TRestRawSignal if by generic you mean that they could operate on Short_t and Double_t types, this could be done also inside TRestRawSignal.

Perhaps, a separate class TRestRawSignalTools? could be constructed, where their methods receive as input std::vector <T>, and then TRestRawSignal just inherits from this class. The present methods could just call the new implemented methods with either fData or fHDData as arguments.

[juanangp]

We discussed this already with @lobis at some point. It is true this fact is missing. We discussed the idea about having another vector inside TRestRawSignal, for example std::vector <Double_t> fHDData; which is usually empty, but that we may fill with a given method. Once we identify this vector has been filled, the methods inside TRestRawSignal they can switch to operate on the high precision version.

We should try to find a solution that stays on the domain of rawlib.

I think that to populate TRestRawSignal with a std::vector <Double_t> fHDData; is not the way to go. I think TRestRawSignal should only contain methods and containers related to raw signals, but it shouldn't contain any signal processing.

Perhaps, a separate class TRestRawSignalTools? could be constructed, where their methods receive as input std::vector <T>, and then TRestRawSignal just inherits from this class. The present methods could just call the new implemented methods with either fData or fHDData as arguments.

If you want to keep generic methods inside rawlib that's fine, but note that these methods cannot be used out of rawlib scope, e.g. detectorlib.

[lobis]

We discussed this already with @lobis at some point. It is true this fact is missing. We discussed the idea about having another vector inside TRestRawSignal, for example std::vector <Double_t> fHDData; which is usually empty, but that we may fill with a given method. Once we identify this vector has been filled, the methods inside TRestRawSignal they can switch to operate on the high precision version. We should try to find a solution that stays on the domain of rawlib.

I think that to populate TRestRawSignal with a std::vector <Double_t> fHDData; is not the way to go. I think TRestRawSignal should only contain methods and containers related to raw signals, but it shouldn't contain any signal processing.

Perhaps, a separate class TRestRawSignalTools? could be constructed, where their methods receive as input std::vector <T>, and then TRestRawSignal just inherits from this class. The present methods could just call the new implemented methods with either fData or fHDData as arguments.

If you want to keep generic methods inside rawlib that's fine, but note that these methods cannot be used out of rawlib scope, e.g. detectorlib.

I also think that in general data classes should be decoupled from analysis methods as much as possible (and especially from drawing methods). I also don't think we need classes for everything, a few functions in an appropiately named namespace should be enough. In general I think we should use namespaces more in the framework, instead of relying on prepending long prefixes to the classes. e.g. TRestRawSignalEvent vs REST::Raw::SignalEvent.

I guess we could define these methods in the framework itself, or define them in rawlib and add the option to compile detectorlib with rawlib support (not sure if this exists already, but some other lbiraries already do this, such as geant4lib with detectorlib support). I don't like either of those options, but I cannot think of better alternatives.

[jgalan]

I agree with those statements, but not with the latest one. We should keep libraries independent one from each other, all inter-dependencies should be kept in connectors lib. There is already a dependency between geant4lib and detector lib? I was not aware ... this should not happen.

There is no need for those methods to be in detectorlib. The signal processing methods are clearly in the domain of rawlib. We should not start to mix things. The function of each library should be pretty clear.

[lobis]

I agree with those statements, but not with the latest one. We should keep libraries independent one from each other, all inter-dependencies should be kept in connectors lib. There is already a dependency between geant4lib and detector lib? I was not aware ... this should not happen.

There is no need for those methods to be in detectorlib. The signal processing methods are clearly in the domain of rawlib. We should not start to mix things. The function of each library should be pretty clear.

Yes, I misremembered, actually what exists is a optional dependance between rawlib and detectorlib already! (via directive etc. #ifdef REST_DetectorLib).

[jgalan]

Yes! Perhaps those pieces of code would better find the way to settle in detectorlib by creating new TRestDetectorRecoverSignalProcess andTRestDetectorChannelActivityProcess, since they need the TRestDetectorReadout.

In that sense it is clear that the detector channel activity will be based on physical readout, while the raw channel activity will be based on daq channels.

Just fixed at rest-for-physics/rawlib#92

[jgalan]

For drawing methods I agree that we should have a common classes directly inside the framework. Then, those methods would be inherit by the existing event types. We could have TRestHitsDrawing and TRestSignalDrawing where we define limits, and common drawing routines, that can be re-exploited by TRestRawSignalEvent and TRestDetectorSignalEvent, or TRestDetectorHitsEvent and TRestGeant4Event. There should be an issue dedicated to this.