Skip to content

Conversation

@RealAurio
Copy link
Contributor

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.

…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.
Copy link
Member

@anthoak13 anthoak13 left a 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

Copy link
Member

@anthoak13 anthoak13 left a 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)
Copy link
Member

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

Copy link
Contributor Author

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));
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 18 to 21
TString fInputBranchName;
TString fOutputBranchName;
TString fRawEventBranchName;
TString fEventBranchName;
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@RealAurio RealAurio left a 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...

Comment on lines +57 to +61
// 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;
Copy link
Contributor Author

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.

Comment on lines 40 to 42
#pragma link C++ class AtPatternModification + ;
#pragma link C++ class AtBraggCurveFinder + ;
#pragma link C++ class AtPatternModificationTask + ;
Copy link
Contributor Author

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.

@RealAurio RealAurio force-pushed the experimentalBraggCurve branch from 69ab616 to 491c91e Compare September 5, 2025 08:52
Copy link
Member

@anthoak13 anthoak13 left a 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!

@anthoak13 anthoak13 merged commit 7ff3909 into ATTPC:develop Sep 5, 2025
2 checks passed
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.

2 participants