-
Notifications
You must be signed in to change notification settings - Fork 20
Implementation of the algorithm to find the experimental Bragg curves from the AtTracks. #257
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
Conversation
…of (range, eLoss). Added a base class for possible classes that modify a given AtPatternEvent. Added the task to apply AtPatternModifications to the AtPatternEvents. Oops. Added the option to pass AtEvent and AtRawEvent to the AtPatternModification. Added a virtual method to the base class AtPattern that computes the distance between 2 points along the pattern. Implemented it for the case of AtPatternLine. Implemented a virtual method in base class AtPattern to obtain the parameter at a given point of the AtPattern. Modified the AtPSAHitPerTB. Forgot to change version number. Implemented an AtPatternModification which extracts the pair of values for the Bragg curve reconstruction and saves them in the AtTracks. The AtPatternModifications need to be able to write to disk. Added quick test macros to check if this feature works so far. Generated the Bragg curve histogram and saved it in a new BraggCurve struct in AtTrack. Added an AtTab to visualize the Bragg curves of the AtTacks. Implemented the smoothing of the Bragg curves.
anthoak13
left a comment
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 want to tweak the interface for pattern modification and the associated task
…ges were needed in AtBraggCurveFinder.
…aybe something else, but it has been 2 weeks and I do not remember...
… AtPatternEvent with the modified one and other changes.
anthoak13
left a comment
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 this looks good with a few tiny comments. The big one is I don't remember the discussion on moving the brag-curve specific parts to their own derived class.
| @@ -0,0 +1,17 @@ | |||
| #include "AtPatternModification.h" | |||
|
|
|||
| void AtPatternModification::ModifyPatternEvent(AtPatternEvent *patternEvent, AtRawEvent *rawEvent, AtEvent *event) | |||
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 either check for null pointers or we should pass AtPatternEvent as a reference rather than a pointer
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.
|
|
||
| // Iterate over the original tracks and store the modified ones. | ||
| for (auto track : tracks) | ||
| modifiedTracks.push_back(GetModifiedTrack(&track, rawEvent, event)); |
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.
Since we just have references anyway, GetModifiedTrack should probably take a reference rather than a pointer.
This use makes me think it should be a constant reference as well (we are returning a new track)
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.
Yeah, initially I just modified the AtTrack, but since we now want to keep the original AtPatternEvent, I can't just modify them.
Also, by returning a different AtTrack, this would allow this function to get a normal AtTrack and return a derived one, in the future when we implement derived AtTracks. But as mentioned earlier, this gave me problems, so for now I'll just work with normal AtTracks.
I will make it so that the input is a const reference to the original AtTrack.
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.
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| class AtPatternModificationTask : public FairTask { |
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 have a doxygen comment explaining this copies event to new branch and modifies each track.
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.
Will add.
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.
| TString fInputBranchName; | ||
| TString fOutputBranchName; | ||
| TString fRawEventBranchName; | ||
| TString fEventBranchName; |
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 have default values for input and output branch name, or those should required parts of the constructor. The class doesn't make sense as an object without them set. Best practices has an object functional an initialization, then use function calls to modify behavior
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 have already commented on this, but forgot to press publish review.
I see in a lot of tasks, the default values are given in the constructor of the task, so that's why I put those there. In any case, I will also put the defaults here too even if it's redundant.
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.
RealAurio
left a comment
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.
My comments that I forgot to submit...
| // Obtained in AtPatternModification | ||
| // Vector of pair of values for the Bragg curve of the track (archLength[mm], eLoss[a.u.]). | ||
| std::vector<std::pair<Double_t, Double_t>> fBraggCurveValues; | ||
| // Container for the Bragg curve integrated over the binning. | ||
| BraggCurve fBraggCurve; |
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.
Yeah, maybe makes sense. AtPatternEvent should still work with these I guess.
| #pragma link C++ class AtPatternModification + ; | ||
| #pragma link C++ class AtBraggCurveFinder + ; | ||
| #pragma link C++ class AtPatternModificationTask + ; |
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.
AtPatternModification and AtBraggCurveFinder can be - !, but AtPatternModificationTask needs to be +, or else it does not compile. Also, other tasks are saved as + here.
69ab616 to
491c91e
Compare
anthoak13
left a comment
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 this is ready to merge!
Added container in AtTrack in order to store the experimental values of (range, eLoss).
Added a base class for possible classes that modify a given AtPatternEvent.
Added the task to apply AtPatternModifications to the AtPatternEvents.
Added the option to pass AtEvent and AtRawEvent to the AtPatternModification.
Added a virtual method to the base class AtPattern that computes the distance between 2 points along the pattern. Implemented it for the case of AtPatternLine.
Implemented a virtual method in base class AtPattern to obtain the parameter at a given point of the AtPattern.
Modified the AtPSAHitPerTB.
Implemented an AtPatternModification which extracts the pair of values for the Bragg curve reconstruction and saves them in the AtTracks.
Added quick test macros to check if this feature works so far.
Generated the Bragg curve histogram and saved it in a new BraggCurve struct in AtTrack.
Added an AtTab to visualize the Bragg curves of the AtTacks.
Implemented the smoothing of the Bragg curves.