Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding generic methods for signal analysis inside framework #379

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

juanangp
Copy link
Member

@juanangp juanangp commented Feb 15, 2023

juanangp Large: 785 Powered by Pull Request Badge

New namespace for generic signal analysis inside framework, it implements different methods:

  • Different methods for baseline calculation
  • Signal smoothing
  • Methods for signal analysis (risetime, width, points over threshold, triple max, integral,...)
  • Methods for signal fitting (gaus, landau, aget)

Related PR:

@jgalan
Copy link
Member

jgalan commented Feb 21, 2023

I understand it is much cleaner to create this dedicated class, however, I still believe the right place for it is at the rawlib.

Only in rawlib it will make sense to calculate baselines, or does it make sense in any other existing or future library?

Thus, I suggest to rename this class to TRestRawSignalAnalysis and place it at rawlib.

@jgalan
Copy link
Member

jgalan commented Feb 21, 2023

If new types (other than Short_t) are introduced for rawsignals, those types should be added somehow also inside rawlib.

Please, share in case that you foresee another use-case where you will need a library for raw signal processing that it is outside rawlib.

@juanangp
Copy link
Member Author

Please, share in case that you foresee another use-case where you will need a library for raw signal processing that it is outside rawlib.

I think this topic is discussed on #353.
Personally, I think that signal analysis methods should be generic and doesn´t have to be linked to any library. From my understanding rawlib holds raw signal data but it doesn't mean that all the signal processing has to be done in this library.
For instance, I was attemting to perform the analysis of the smoothed signal which was difficult with the current code design, this is actually implemented on under a related PR rest-for-physics/connectorslib#27.
In addition detectorlib currently holds several methods for signal processing, despite signal smoothing or baseline calculations, there are several methods such as GetMaxLandau(), GetMaxGauss() or GetMaxAget() that are used inside TRestDetectorSignalToHitsProcess.
On the other hand, I don't see any issue to perform the signal analysis on a ´TRestDetectorEvent´.

@juanangp juanangp marked this pull request as ready for review April 3, 2023 17:47
@juanangp juanangp requested review from jgalan, lobis and a team April 3, 2023 17:47
@juanangp juanangp changed the title WIP: Adding generic methods for signal analysis inside framework Adding generic methods for signal analysis inside framework Apr 3, 2023
@juanangp juanangp requested a review from a team April 14, 2023 09:29
@nkx111
Copy link
Member

nkx111 commented May 15, 2023

If I remember correctly you cannot write the implementation of the template methods separately in the cxx file, unless you #include this cxx file like a .h file. I don't know why it works as the pipeline is green...

@juanangp
Copy link
Member Author

If I remember correctly you cannot write the implementation of the template methods separately in the cxx file, unless you #include this cxx file like a .h file. I don't know why it works as the pipeline is green...

As far as I know, template specializations can be defined in the source file https://stackoverflow.com/questions/115703/storing-c-template-function-definitions-in-a-cpp-file
I think it is cleaner to do the implementation inside the source file. The code compiles and works properly.

///

template <typename T>
std::vector<std::pair<Float_t, Float_t> > TRestPulseShapeAnalysis::GetPointsOverThreshold(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could return std::vector<std::pair<Int_t, T> >?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is related to rest-for-physics/rawlib#100 (comment)
I will try to implement a more generic method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants