-
Notifications
You must be signed in to change notification settings - Fork 2
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 new analysis TRestTrackAlphaAnalysisProcess #13
Conversation
macros/REST_Track_PlotAlphaAna.C
Outdated
|
||
} | ||
|
||
TPad *DrawHits ( TRestDetectorHitsEvent *hitsEvent, const TVector3 &origin, const TVector3 &end, TVector3 &max, TVector3 &min, TVector3& nBins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good that Drawing methods would be common inside TRestDetectorHitsEvent
.
macros/REST_Track_PlotAlphaAna.C
Outdated
|
||
} | ||
|
||
TPad *DrawPulses ( TRestRawSignalEvent *rawSignalEvent, TRestDetectorReadout *fReadout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be implemented at TRestDetectorSignalEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that I need the readout info to distinguish between X and Y tracks and it is not straithforward to get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hits at TRestDetectorSignalEvent
know about the readout, and also are capable to identify if they are X, Y type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are TRestRawSignalEvents, I have already implemented a method to plot a vector of sIDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought some pulse drawing methods at the detector level were alreaqdy implemented by you at TRestDetectorReadoutEventViewer
macros/REST_Track_PlotAlphaAna.C
Outdated
const int offset=10;//bins | ||
|
||
|
||
TPad *DrawTrack ( TRestTrackEvent *trackEvent, const TVector3 &origin, const TVector3 &end, const TVector3 &max, const TVector3 &min, const TVector3& nBins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps better to implement inside TRestTrackEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is difficult to get the histogram boundaries, so I pass several TVector3 for the boundaries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but anyway it could be implemented at TRestTrackEvent and the macro would just call to this method?
See for example SetBoundaries
method at TRestDetectorHitsEvent
. https://sultan.unizar.es/rest/TRestDetectorHitsEvent_8cxx_source.html
Of course, those methods could be even further promoted to a higher class, it would be good to have dedicated drawing classes for hits and for time signals that centralize that kind of staff.
/// | ||
/// TRestTrackAlphaAnalysisProcess.cxx | ||
/// | ||
///_______________________________________________________________________________ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add the official headers. Add also some documentation, include figures if possible.
See for example https://sultan.unizar.es/rest/classTRestDetectorSignalToRawSignalProcess.html
|
||
debug<<"Track length "<<length<<" angle: "<<angle<<endl; | ||
|
||
// A new value for each observable is added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document observables in documentation
|
||
//______________________________________________________________________________ | ||
void TRestTrackAlphaAnalysisProcess::InitFromConfigFile() { | ||
fTrackBalance = StringToDouble(GetParameter("trackBalance", "0.65")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to implement this method. Simply remove completely InitFromConfigFile
from TRestTrackAlphaAnalysisProcess
.
Then, TRestEventProcess::InitFromConfigFile
will be responsible to identify a fDummyVariable
with the parameter given in the config file dummyVariable
.
In the case of trackBalance
you just need that a data member named fTrackBalance
exists.
The declaratio at the header would look as:
/// A parameter to define the track balance ... bla bla bla
Double_t fTrackBalance = 0.65;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the methods LoadDefaultConfig()
, LoadConfig()
, TRestTrackAlphaAnalysisProcess(char* cfgfile)
are not needed either.
inc/TRestTrackAlphaAnalysisProcess.h
Outdated
|
||
protected: | ||
// add here the members of your event process | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods to be documented at CXX.
inc/TRestTrackAlphaAnalysisProcess.h
Outdated
/// | ||
/// TRestTrackAlphaAnalysisProcess.h | ||
/// | ||
///_______________________________________________________________________________ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use official headers
Not sure how it is related to #31, perhaps you refer to the wrong PR? |
macros/REST_Track_PlotAlphaAna.C
Outdated
#include <TRestRawSignalEvent.h> | ||
|
||
#include "dirent.h" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should not be implemented at the level of tracklib
. If REST is not compiled with detectorlib
or rawlib
there will be problems when this macro is being loaded in restRoot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, you are right.
Do you prefer to move to general macros directory? I don't know what is the best strategy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libraries design is to focus on independent development of libraries. So that a particular library keeps a minimum of dependencies with other libraries. I am sure it is possible to think a way to keep that strategy alive.
In the case this is not possible, and using rawlib
routines inside the tracklib
is a must (although I would prefer to find a way to avoid) then the code instantiating rawlib
must be encapsulated using a compilation flag, that could be created for example at cmake/MacroRootDict.cmake
at COMPILEDIR
, and define a #ifdef RESTLIB_RAW
, not sure if this already exist. @nkx?
However, I would prefer that such a possibility does not exist, the present structure is the result of an effort to organise the code project, and I believe keeping isolated developments at the libraries as they are is the best to avoid future chaos. I understand this requires an effort, but I am open to discuss about possible solutions for the present implementation.
Keep present that if you add your macro to the tracklib
pipeline, where only track routines are tested, the execution will fail.
https://gitlab.cern.ch/rest-for-physics/tracklib/-/pipelines/3637513
At the rawlib there are already a few tests running that do not need other libraries, we should push for something similar at tracklib
https://gitlab.cern.ch/rest-for-physics/tracklib/-/pipelines/3637513
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding it to the framework domain is also not desirable. The framework should know nothing about signal processing, physical track event reconstruction, detectors, etc. It should only implement basic analysis routines not connected to those domains. Such as fitting analysis tree observables, etc. This is discussed in the paper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add dependency we can just call:
set(deps detector geant4 track raw)
COMPILELIB(deps)
Currently only connectorslib has dependency on others. Others are all independent. So it would be strange to make tracklib dependent on rawlib.
macros/REST_Track_PlotAlphaAna.C
Outdated
return fPad; | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably need to be implemented at TRestTrackEvent
.
macros/REST_Track_PlotAlphaAna.C
Outdated
endYZGr->Draw("LP"); | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be implemented at TRestTools
? There is no documentation, what it is doing this method? Perhaps there is already a similar method at TRestTools
doing what is doing this one?
… is performed in a previous step
…p of unused classes
…e tsp problem BruteForce and NearestNeighbour. Clean up of unused classes.
inc/TRestTrackLineAnalysisProcess.h
Outdated
|
||
#include "TRestEventProcess.h" | ||
|
||
//! A process to analyze alpha tracks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! A process to identify and extract the track ends in linear approximation?
inc/TRestTrackLineAnalysisProcess.h
Outdated
void Initialize(); | ||
|
||
protected: | ||
/// A parameter to define the track balance, otherwise the event is rejected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear, when the event is rejected?
inc/TRestTrackLinearizationProcess.h
Outdated
EndPrintProcess(); | ||
} | ||
|
||
// This function performs the track linearization towards a given number of nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description should be written in the implementation, at the cxx file.
|
||
ClassImp(TRestTrackLineAnalysisProcess); | ||
|
||
//______________________________________________________________________________ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the good formatting. Using REST_MakeProcess
will produce the standard formatting.
////////////////////////////////////////////////////////////////////////// | ||
/// The TRestTrackLineAnalysis process is meant to analyze lienar tracks, | ||
/// such as alpha tracks. It should retreive the information after performing | ||
/// TRestTrackLinearization process, for the time being only works when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it happens if TRestTrackLinearizationProcess
was not used earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, is not required to use TRestTrackLinearizationProcess
before. It should work as soon as the reduced hits are ordered. I am going to change the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. It is this process doing a linear fit to the hits? In that case the hit order would also be not relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hit order is required in the sense that it should follow the minimum path, this is guaranteed after performing TRestTrackLinearizationProcess
. It can be also reached using TRestTrackPathMinimizationProcess
after a TRestTrackReductionProcess
.
However, at this stage the hits ordering doesn't require to have any preferred direction e.g. a hit ordering of 1->2->3->4->5
should give same solution as an inverted ordering 5->4->3->2->1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, we should probably introduce a mechanism that allows us to do something like ProcessRequired("TRestTrackPathMinimizationProcess")
so that if the process does not exist earlier in the data chain it will be returning an error.
Another potential issue, perhaps @nkx111 sees an easy way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue about it rest-for-physics/framework#182
/// following the minimum path. It assumes that only one alpha track | ||
/// is present, using the parameter fTrackBalance to define the minimun energy | ||
/// fTrackBalance*totalEnergy of the total XZ or YZ energy to assume a single | ||
/// track event, if the condition is not fulfilled the event is rejected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to do not reject events. If it is not a good event, then we will be able to discriminate it later using filters at the analysisTree
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the parameted fTrackBalance
has been removed, now we have 2 observables instead trackBalanceX
and trackBalanceY
//______________________________________________________________________________ | ||
TRestEvent* TRestTrackLinearizationProcess::ProcessEvent(TRestEvent* evInput) { | ||
fTrackEvent = (TRestTrackEvent*)evInput; | ||
fOutTrackEvent->SetEventInfo(fTrackEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary anymore, event header transfer is implemented at:
void TRestEventProcess::BeginOfEventProcess(TRestEvent* inEv) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetEventInfo
has been removed
////////////////////////////////////////////////////////////////////////// | ||
/// The TRestTrackLinearization process is meant to analyze linear tracks, | ||
/// such as alpha tracks. It should retreive the information after performing | ||
/// TRestTrackLinearization process, not implemented for XYZ tracks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetHitsProjection
was implemented time ago to reconstruct the profile of a physical track event along the track. The track could be a complex electron track event (no need to be linear). I understand this process is doing now something else, I could enter in the details of how this is being used here, but I dont see how this is required to perform the fitting to the raw hits.
The process producing a track profile could be probably named TRestTrackProfileProcess
, generating a std::vector
or a TGraph
with N
points where the hits energy have been projected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand this comment. Do you want to rename TRestTrackLinearizationProcess
to TRestTrackProfileProcess
?
Perhaps the naming of GetHitsProjection
function is not adequate? I can rename the function. This function is actually performing the track linearization.
Or you want to split the process in two different processes one doing the linear fit or profile and another process doing the average of the closest hits? Also, I don't understand why do you want to store the results as an std::vector
or TGraph
since I believe is better to store as TRestVolumeHits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discuss this online
|
||
// Return the index with the shortest path solving Travelling Salesman Problem (TSP) using | ||
// brute force, algorithm complexity is n!, the minimum path is guarantee | ||
//______________________________________________________________________________ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the standard format for method documentation
src/TRestTrackViewerProcess.cxx
Outdated
/// | ||
void TRestTrackViewerProcess::DrawOriginEnd(TPad* pad, std::vector<TGraph*>& origin, | ||
std::vector<TGraph*>& end, std::vector<TLegend*>& leg) { | ||
for (int i = 0; i < 2; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should check if origin
, end
and leg
got exactly 2-elements. I would allow to do not check this if the method were private
and it is clear that the only case is that it will contain 2-elements. But the method was declared public
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/TRestTrackViewerProcess.cxx
Outdated
/// \brief Get origin and end of the track if trackLineAnalysis have | ||
/// been performed | ||
/// | ||
void TRestTrackViewerProcess::GetOriginEnd(TRestAnalysisTree* anaTree, std::vector<TGraph*>& origin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it receives a pointer to TRestAnalysisTree
? The process will always have access to the analysis tree, i.e. using this->GetFullAnalysisTree()
or this->GetAnalysisTree()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is a method to be implemented at TRestTrackEvent
, or event at a new class TRestTrackPainter
that implements statics methods, then TRestTrackEvent
could inherit from TRestTrackPainter
, the methods at TRestTrackPainter
would get the pointer to the TRestTrackEvent
, but I see it harder with TRestAnalysisTree
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the function is declared static
, so it need a TRestAnalysisTree
passed to the function. I prefer to use static
functions in this case because it allows me to use elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the benefit of using static functions, just that perhaps should be placed at a common class contextualizing it, such as TRestTrackPainter
. For me, it makes sense to associate a GetOriginEnd
to a TrackEvent
, but not to the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps a TRestTrackAnalysis
that contains a static method GetOriginEnd
implementing an algorithm that extracts origin/end from the track, but not from the analysis tree. I would do the other way around. I would use the method to fill the analysis tree inside the process implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that most of the static methods should be implemented in a dedicated class
, perhaps in this case would be better to use a namespace
.
I like the idea to have different namespace
such as TRestTrackPainter
and TRestTrackAnalysis
. However, I think that this approach should be more generic:
TRestTrackPainter
should implement all the track drawing methods.TRestTrackAnalysis
should implement all the track analysis methods.
We should probably use a similar schema for all the libraries by defining dedicated TRestXXXPainter
and TRestXXXAnalysis
namespaces in every single library.
However, this approach would require several changes in the code that cannot be addressed in this PR. I can give a hand with the migration, but it would require a lot of work. Let me know what do you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds good. If someone is leading that, I will also give a hand with the migration.
For this PR I think we need to chat online about the real need to request analysisTree variables in that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am working on that, TRestTrackViewerProcess
can call a static function on TRestTrackLineAnalysisProcess
to calculate the origin and the end of the track.
Later on we can migrate all the static functions to TRestTrackPainter
and TRestTrackAnalysis
, or do yo prefer to do in this PR just with few static functions mentioned before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that could be done later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have created an issue about this topic
src/TRestTrackViewerProcess.cxx
Outdated
std::vector<std::string> eName = {"endX", "endY", "endZ"}; | ||
|
||
for (int i = 0; i < 3; i++) { | ||
std::string fName = "alphaTrackAna_" + oName[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alphaTrackAna_
is defined by the user at the RML, then the name could be anything else, isnt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this observable was extracted from TRestTrackAnalysisProcess
one could perhaps retrieve the process name as follows:
fRunInfo->GetMetadataClass("TRestTrackAnalysisProcess")->GetName();
However, it might happen that there are two classes TRestTrackAnalysisProcess
inside the data processing chain.
Not sure if there is a better way @nkx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that GetObservableValue
is used somewhere else in a similar way.
I can think in some analysis processes that may use some variables from the analysis tree as input. So, I don't know what is the best solution here. As far as I understand the observables are always retreived from the rml
.
Perhaps we should create an issue about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I dont think it is good to hardcode the process name, since the user might change that value. On top of that, as I said earlier, it might happen that exist two TRestRawSignalAnalysisProcess
inside the data processing chain. Which of them TRestDetectorSingleChannelAnalysisProcess
would use?
On the other hand, the process TRestDetectorSingleChannelAnalysisProcess
uses a TRestDetectorSignalEvent
, so I believe the max_amplitude
, thr_integral
and PeakAmplitudeIntegral
maps could be calculated directly inside the process without the need to retrieve them from a previous analysis. Isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just created issue rest-for-physics/detectorlib#47
…unctions to static
…de TRestTrackEvent and TRestTrack
Please, before merging increase the library version, tag it and add the release notes. |
New analysis have been added to tracklib to analyze alpha tracks, which was developed for AlphaCAMM.
A macro has been added to display the analysis results.
Summary of latest changes as requested in this PR:
TRestTrackViewerProcess
for track visualization.TRestTrackEvent
TRestTrackAlphaAnalysisProcess
toTRestTrackLineAnalysisProcess
TRestTrackLinearizationProcess
, which is meant for track linearization.TRestTrackReductionProcess
andTRestTrackPathMinimizationProcess