Tentative changes for new base class AtFittedTrack. #254

anthoak13 · 2025-07-21T15:51:58Z

No description provided.

anthoak13 · 2025-08-06T15:02:09Z

AtData/AtFitResult.h

+   // Vector to store the results for all different fits done to all tracks in the event.
+   ResultsMap fResults;


Needs better comment. What is the map? Like what is the integer? How does it connect to other ints? I don't remember already lol

You are right, the comment could be better.

The integer in this map connects to the fTrackId for which the fits were performed, while the vector of AtFitTrackResult is the collection of fit results of all fits performed for the same track (for example, take one track, assume it could be either proton, deuterium or tritium, and check all 3 cases). Will update this comment.

Done in 3rd commit after this one.

anthoak13 · 2025-08-06T15:04:30Z

AtData/AtFittedTrack.h

+   struct Kinematics {
+      Double_t kineticEnergy{-1};    // Kinetic energy
+      Double_t theta{-1};            // Theta scattering angle
+      Double_t phi{-1};              // Phi scattering angle
+      Double_t kineticEnergyXtr{-1}; // Extrapolated kinetic energy
+      Double_t thetaXtr{-1};         // Extrapolated theta scattering angle
+      Double_t phiXtr{-1};           // Extrapolated phi scattering angle
+   };


I think this should just be KE, theta, phi. Then we can store two coppies of it. Extr and non-extrap.

Really, this could also just be momentum too. Or some equivalent vector if we don't want to have a new struct?

In the end, we are interested in getting kinetic energy and angles, but I could also store a TLorentzVector.

Also, I initially saved 2 copies of Kinematics, one for extr and one for non-extr. I guess I will change it back to that.

Separated the kinematics in 2 different members, one for non-extrap and one for extrap. This is in 4th commit after this one

AtData/AtFittedTrack.h

anthoak13 · 2025-08-06T15:07:32Z

AtData/AtFittedTrack.h

-   Float_t fEnergyXtr{0};
-   Float_t fExcitationEnergyXtr{0};
+   // Track properties.
+   TrackProperties fTrackProperties;


Why is this not a vector but others are vectors?

Others are vectors because we are considering that 1 AtTrack could contain multiple particles, thus multiple entries in the vectors. However, for track properties, these are properties of the track as a whole, so it should only be 1 entry.

Please check the definition of TrackProperties just in case you see a variable that should not live in there based on this.

I think most of the track properties seem to be particle-track related rather than AtTrack related. Like the dedx would not make sense to talk about an entire event. Looking through the list I think I would say all of those parameters are probably particle specific so I would put this as a vector.

Well, it depends. Maybe dEdx is not the best example, since it should be particle specific. But for example, the total charge is meant to be the total charge of the track, being it 1, 2 or 3 particles, same with number of hits, it's meant to be total hits in all the track. The extrapolated distance is meant to be the total distance that needed to be extrapolated from the first hit to the real vertex along the axis of the ATTPC, for the part of the track that comes out of the beam region in case we are talking about the fission pattern.

Meanwhile, real particle vertex would be stored in the fVertices vector, one for each particle.

The ony one that maybe feel out of place to me is the dEdx, but in a case where there is only 1 particle in the track, it can still make sense, and maybe it can simply be ignored in other cases. Maybe track lengths also do not make sense in case that there is more than 1 particle.

AtData/AtFittedTrack.h

anthoak13 · 2025-08-06T15:24:32Z

AtReconstruction/AtFitter/AtFitter.h

+   using TrackResultsSet =
+      std::set<TrackResultPtr, std::function<bool(const TrackResultPtr &, const TrackResultPtr &)>>;
+
+protected:


I don't think it makes much sense to store temporary pointers that users have to remember to manage

Yes, as in #257. Will work on this.

Done in 5th commit after this one.

anthoak13 · 2025-08-06T15:25:57Z

AtReconstruction/AtFitterTask.h

+   void SetRawEventBranch(TString branchName);
+   void SetEventBranch(TString branchName);
+
+   void SetFitResultBranch(TString branchName);


So is AtFitResult actually the equivalent of AtFittedTrack?

No, it was a bad choice of name. It should be FitMetadata. This was already changed to the new naming.

anthoak13 · 2025-08-06T15:27:49Z

AtReconstruction/AtFitterTask.h

-class Track;
-} // namespace genfit

 class AtFitterTask : public FairTask {


Probably needs a very robust doc string about the logic.

Done in 6th commit after this one.

anthoak13 · 2025-08-06T15:28:53Z

AtReconstruction/AtFitterTask.cxx

+   auto fitResult = dynamic_cast<AtFitResult *>(fFitResultArray.ConstructedAt(0));

-   std::cout << " Event Counter " << fEventCnt << "\n";
+   LOG(info) << " AtFitterTask::Exec() : Fitting event " << fEventCnt;


Remove hard coded function name

Done in 5th commit after this one

anthoak13 · 2025-08-06T15:29:04Z

AtReconstruction/AtFitterTask.cxx

-   std::cout << " AtFitterTask:Exec -  Number of candidate tracks : " << tracks.size() << "\n";
+   AtPatternEvent *patternEvent = dynamic_cast<AtPatternEvent *>(fPatternEventArray->At(0));
+   std::vector<AtTrack> &tracks = patternEvent->GetTrackCand();
+   LOG(info) << " AtFitterTask::Exec() : Number of candidate tracks : " << tracks.size();


remove func name

Done in 5th commit after this one

RealAurio

First changes I will work on. I added a couple of questions as to how I should approach this.

RealAurio · 2025-09-01T10:35:00Z

AtData/AtFitTrackResult.h

+/**
+ * Class for storing the result of the fit of an AtTrack from an AtFitter class.
+ */
+class AtFitTrackResult : public TObject {


I think we wanted to name this AtFitMetadata, but for some reason I forgot about this. I can change the naming of the class. However, the now derived class AtMCResult has result in the naming, but I'm afraid of changing the naming in AtMCFitResult since it was an already existing class and would imply changing the convention in a lot of places.

For now, I will change the name of the base class and leave AtMCResult unchanged, but please let me know if you still want to change the naming of AtMCResult.

RealAurio · 2025-09-01T10:36:31Z

AtData/AtFittedTrack.h

+   struct Kinematics {
+      Double_t kineticEnergy{-1};    // Kinetic energy
+      Double_t theta{-1};            // Theta scattering angle
+      Double_t phi{-1};              // Phi scattering angle
+      Double_t kineticEnergyXtr{-1}; // Extrapolated kinetic energy
+      Double_t thetaXtr{-1};         // Extrapolated theta scattering angle
+      Double_t phiXtr{-1};           // Extrapolated phi scattering angle
+   };


In the end, we are interested in getting kinetic energy and angles, but I could also store a TLorentzVector.

Also, I initially saved 2 copies of Kinematics, one for extr and one for non-extr. I guess I will change it back to that.

AtData/AtFittedTrack.h

RealAurio · 2025-09-01T10:39:45Z

AtData/AtFittedTrack.h

-   Float_t fEnergyXtr{0};
-   Float_t fExcitationEnergyXtr{0};
+   // Track properties.
+   TrackProperties fTrackProperties;


Others are vectors because we are considering that 1 AtTrack could contain multiple particles, thus multiple entries in the vectors. However, for track properties, these are properties of the track as a whole, so it should only be 1 entry.

Please check the definition of TrackProperties just in case you see a variable that should not live in there based on this.

AtData/AtFittedTrack.h

RealAurio · 2025-09-01T10:41:58Z

AtData/AtFittedTrack.h

-   inline void SetTrackID(Int_t trackid) { fTrackID = trackid; }
+   void SetTrackID(Int_t trackid) { fTrackID = trackid; }

-   inline void SetEnergyAngles(Float_t energy, Float_t energyxtr, Float_t theta, Float_t phi, Float_t energypra,


RealAurio · 2025-09-01T10:44:08Z

AtData/AtFittedTrack.h

-   const std::tuple<Float_t, Float_t, Float_t, Float_t, Float_t, Float_t, Float_t> GetEnergyAngles();
-   const std::tuple<XYZVector, XYZVector, XYZVector> GetVertices();
-   const std::tuple<Float_t, Float_t, Float_t, Float_t, Float_t, Bool_t> GetStats();
-   const std::tuple<Int_t, Float_t, Float_t, Float_t, std::string, Int_t> GetTrackProperties();
-   const std::tuple<Float_t, Float_t> GetIonChamber();
-   const std::tuple<Float_t, Float_t> GetExcitationEnergy();
-   const std::tuple<Float_t, Float_t, Float_t> GetDistances();


Agreed, will bring these templates back and change the implementation so it adapts to the refactored class.

AtData/AtFittedTrack.h

RealAurio · 2025-09-01T10:49:07Z

AtData/AtTrackingEventOld.h

+class TClass;
+class TMemberInspector;
+
+class [[deprecated]] AtTrackingEventOld : public AtBaseEvent {


I think the new version did in fact not need any new changes, so it remained unchanged and thus it does not appear in this commit (the new AtFittedTrack is still named AtFittedTrack). As for the old one, I had to change AtFittedTrack to AtFittedTrackOld, and stuff like that, so this new class was needed with just that little change.

RealAurio

Some changes

RealAurio · 2025-09-01T14:58:31Z

AtData/AtFitTrackResult.h

+/**
+ * Class for storing the result of the fit of an AtTrack from an AtFitter class.
+ */
+class AtFitTrackResult : public TObject {


Renaming from result to metadata done in next 2 commits.

RealAurio · 2025-09-01T15:13:21Z

AtData/AtFitResult.h

+   // Vector to store the results for all different fits done to all tracks in the event.
+   ResultsMap fResults;


Done in 3rd commit after this one.

AtData/AtFittedTrack.h

anthoak13

I think this looks good? Is there anything else you would want to change before we try and merge?

anthoak13 · 2025-09-03T23:10:46Z

AtData/AtFittedTrack.h

-   Float_t fEnergyXtr{0};
-   Float_t fExcitationEnergyXtr{0};
+   // Track properties.
+   TrackProperties fTrackProperties;


I think most of the track properties seem to be particle-track related rather than AtTrack related. Like the dedx would not make sense to talk about an entire event. Looking through the list I think I would say all of those parameters are probably particle specific so I would put this as a vector.

anthoak13 · 2025-09-03T23:15:14Z

AtData/AtFittedTrack.h

-   const std::tuple<Float_t, Float_t, Float_t, Float_t, Float_t, Float_t, Float_t> GetEnergyAngles();
-   const std::tuple<XYZVector, XYZVector, XYZVector> GetVertices();
-   const std::tuple<Float_t, Float_t, Float_t, Float_t, Float_t, Bool_t> GetStats();
-   const std::tuple<Int_t, Float_t, Float_t, Float_t, std::string, Int_t> GetTrackProperties();
-   const std::tuple<Float_t, Float_t> GetIonChamber();
-   const std::tuple<Float_t, Float_t> GetExcitationEnergy();
-   const std::tuple<Float_t, Float_t, Float_t> GetDistances();


I am happy to keep it this way. Or for functions that partially set the removed member variables we mark them deprecated. Functions that only set removed members are removed?

I'm not sure what the best option is. I think we will break things regardless, but the less the better I suppose. Maybe we try to run some old codes to see how much a pain these different changes are to deal with?

anthoak13 · 2025-09-03T23:16:40Z

AtReconstruction/AtFitter/AtFitter.cxx

+void EventFit::AtFitter::FitEvent(AtTrackingEvent *trackingEvent, AtPatternEvent *patternEvent,
+                                  AtFitMetadata *fitMetadata, AtRawEvent *rawEvent, AtEvent *event)
 {
+   // Extract the candidate AtTracks.


Should we add checks for null pointer here?

This would be something similar as the last change that you asked for in #257. I can add a quick check. If no pattern event, then return.

Also for tracking event actually.

Raw event, event and fit metadata can be nullptr considering how GetFittedTrack is defined.

Done in last commit.

…lysis.

…ously AtFitResult).

…g-formatted.

…tics in 2.

… AtFITTER to EventFit.

…make sense to keep in the base class. Also fixed some serious bugs that I found while actually implementing a fitter class in the braggCurveFitter feature branch.

RealAurio force-pushed the fitterRefactor branch 7 times, most recently from a8c73ed to ff9dec7 Compare July 30, 2025 15:23

anthoak13 marked this pull request as ready for review August 5, 2025 18:30

anthoak13 commented Aug 6, 2025

View reviewed changes

RealAurio reviewed Sep 1, 2025

View reviewed changes

RealAurio force-pushed the fitterRefactor branch 2 times, most recently from b1ba376 to 4e393ef Compare September 3, 2025 10:48

anthoak13 commented Sep 3, 2025

View reviewed changes

RealAurio force-pushed the fitterRefactor branch 2 times, most recently from e16a09f to 4b1fa7e Compare September 8, 2025 08:13

RealAurio added 8 commits September 8, 2025 10:16

Refactored a bunch of the code related to the fitting step of the ana…

3d7023d

…lysis.

Renamed and most changes regarding rebranding of AtFitMetadata (previ…

888d091

…ously AtFitResult).

Last changes, in different commit due to renaming of the file.

43f4ae3

Changes some comments, removed statistics from AtFittedTrack and clan…

d89796f

…g-formatted.

Brought back the old getters and setters, as well as split the Kinema…

ef876d7

…tics in 2.

Changed the interface of AtFitter, changed the name of namespace from…

e4b9e89

… AtFITTER to EventFit.

Last change?

ed58fd9

Very last change?

841e510

RealAurio force-pushed the fitterRefactor branch from 4b1fa7e to 841e510 Compare September 8, 2025 08:17

Removed stuff from AtFitter that ended up being too case specific to …

11b7a6f

…make sense to keep in the base class. Also fixed some serious bugs that I found while actually implementing a fitter class in the braggCurveFitter feature branch.

		// Vector to store the results for all different fits done to all tracks in the event.
		ResultsMap fResults;

Tentative changes for new base class AtFittedTrack. #254

Are you sure you want to change the base?

Tentative changes for new base class AtFittedTrack. #254

Uh oh!

Conversation

anthoak13 commented Jul 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RealAurio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RealAurio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!