Fwd Track matching to FCS clusters #398

jdbrice · 2022-09-13T19:31:26Z

Akio's Fwd track match maker for FCS clusters

Depends on
#396
and
#397

akioogawa · 2022-09-14T02:11:26Z

I think StFwdTrack and StFwdTrackCollection to StEvent should be in asap (396 and 397). But don't we need a formal code review for the new StFwdTrackMaker & StFcsTrackMatchMaker? Or are we doing the code review at this PR right here now? Also StFcsTrackMatchMaker is sort of work in progress and I have not yet "cleaned up" for PR, thinking PR for it will come after all forward tracking code merged to STAR main git... I'll have closer look and will clean it up as needed here in the PR I guess.

plexoos · 2022-09-14T15:28:17Z

But don't we need a formal code review for the new StFwdTrackMaker & StFcsTrackMatchMaker?

StFwdTrackMaker was initially reviewed more than two years ago. You can see the package under StRoot/

Or are we doing the code review at this PR right here now? Also StFcsTrackMatchMaker is sort of work in progress and I have not yet "cleaned up" for PR, thinking PR for it will come after all forward tracking code merged to STAR main git... I'll have closer look and will clean it up as needed here in the PR I guess.

It can be reviewed in this PR if that is preferred. If you have someone in mind who you would like to see as a reviewer, the request can be done via the PR page. I'd also recommend to add the respective change in .github/CODEOWNERS directly on this PR branch (it would make it somewhat more formal). If there are no volunteers I can review but the first look reveals that the code needs more work indeed, the maker leaks memory like a sieve. I'd suggest to mark this PR as draft for now.

jdbrice · 2022-09-15T13:47:41Z

Right, I was expecting that this PR would initiate the official review (maybe now or once #396 and #397 are merged)
I will provide reviewer suggestions asap.
@plexoos do we have a list of possible reviewers. Maybe the code owners file is a good list of acceptable people?

plexoos · 2022-09-15T14:14:19Z

Maybe the code owners file is a good list of acceptable people?

Yes, of course. But don't hesitate to invite new members from the collaboration too (I thought you had a helper student/postdoc interested in the forward analysis). From the NPPS side I think Victor @perevbnlgov is back, in case you need a pair of experienced eyes

starsdong · 2022-09-15T15:29:38Z

Yes, we should try to get 2-3 reviewers to proceed with the official review for this new maker. Will follow on this asap. Thanks

starsdong · 2022-09-15T18:11:47Z

Added Zilong Chang as a reviewer for the new maker.

starsdong · 2022-09-16T12:19:31Z

Zilong and Victor are invited to review the new StFcsTrackMatchMaker in this PR.
Zilong, Victor, thank you for agreeing to help on this. Your timely comments / approvals to this PR would be much appreciated. Thanks

akioogawa · 2022-09-21T13:53:46Z

.github/CODEOWNERS has @fisyak who does not exist??? I guess this is nothing to do with line I've added...

plexoos · 2022-09-21T13:58:05Z

.github/CODEOWNERS has @fisyak who does not exist???

For some reason he does not want to join

perevbnlgov · 2022-09-22T01:20:12Z

I do not know how speed is important there. If it is important then it could
be optimized. In proposed code:

  StThreeVectorD xyz=mFcsDb->getStarXYZfromColumnRow(det,clu->x(),clu->y());
  double dx = xyz.x() - proj.x();
  double dy = xyz.y() - proj.y();
  double dr = sqrt(dx*dx + dy*dy);

//I removed debug here
if(dr<mMaxDistance[ehp]){
if(ehp==0){trk->addEcalCluster(clu);}
if(ehp==1){trk->addHcalCluster(clu);}
clu->addTrack(trk);
nMatch[ehp]++;
}
But the following will be faster because for the wrong case dx or dy is already too big

  double maxDr = mMaxDistance[ehp])
  double dx = fabs(xyz.x() - proj.x()); if (dx)> maxDr) break;
  double dy = fabs(xyz.y() - proj.y()); if (dy > maxDr) break;
  double dr = sqrt(dx*dx + dy*dy);     if (dr > maxDr) break;

  if(ehp==0){trk->addEcalCluster(clu);} else {trk->addHcalCluster(clu);}
  clu->addTrack(trk);
  nMatch[ehp]++;

klendathu2k · 2022-09-22T16:02:22Z

Looking at this code I see a dependence on genfit objects which should not be stored in StEvent. This issues has already been noted in PR#397:
#397 (comment)

klendathu2k · 2022-09-22T16:16:37Z

StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.cxx

+
+    //North or south from track
+    int ns=0;
+    if(trk->mProjections[0].XYZ.x()>0.0) ns=1;


mProjections should not be public on StFwdTrack.

What is wrong in this case by making it public on StFwdTrack?

I believe our coding standards require member variables to not be publicly accessible.

The coding guidelines suggest to make data members private except in structs https://star-bnl.github.io/star-coding-guidelines/codingguide.xml#Access_Control
It also looks like Akio has fixed this in the followup commit. However, I don't see where the StFwdTrack::mProjections are being set in the submitted code. There appears to be no setter.

klendathu2k · 2022-09-22T16:20:00Z

StRoot/StEvent/StFwdTrack.h

+
+class StFcsCluster;
+
+struct StFwdTrackProjection {


This should be a class, rather than a struct, and should properly protect its data. Also, should follow STAR naming conventions. "cov" --> mCov. XYZ --> mXyz.

klendathu2k · 2022-09-22T16:28:58Z

StRoot/StEvent/StFcsCluster.h

+    StPtrVecFwdTrack& tracks();
+    const StPtrVecFwdTrack& tracks() const;
+    void addTrack(StFwdTrack* p);
+    void sortTrackByPT();


Why do you need to have sortTrackByPT() defined on StFcsCluster? If you need the tracks sorted in some order,
just sort them... you already have the reference to the track vector...
void inSomeAnalysis() {
auto& tracks = mCluster->tracks();
std::sort(tracks.begin(), tracks.end(), [](StFwdTrack* a, StFwdTrack* b) {
return b->momentum().perp() < a->momentum().perp();
});
}

zlchang · 2022-09-22T17:01:55Z

StRoot/StEvent/StFcsCluster.cxx

+}
+
+void StFcsCluster::sortTrackByPT() {
+    std::sort(mTracks.begin(), mTracks.end(), [](StFwdTrack* a, StFwdTrack* b) {


Suggested change

std::sort(mTracks.begin(), mTracks.end(), [](StFwdTrack* a, StFwdTrack* b) {

std::sort(mTracks.begin(), mTracks.end(), [](const StFwdTrack* a, const StFwdTrack* b) {

Add const qualifier to the compare function

StRoot/StEvent/StFwdTrack.cxx

akioogawa · 2022-09-23T14:36:57Z

(On dr calc in StFcsTrackMatchMaker.cxx) I do not know how speed is important there. If it is important then it could be optimized.

Speed is important. But speed gain from the suggested code seems minimal, and modified code seems bad to me. But I need dR for debug and histograms... Please suggest other solution if you agree with me that the modified code looks bad.

StFcsTrackMatchMaker::Finish(){
if(mEpdgeo) delete mEpdgeo;
causes
warning: deleting object of polymorphic class type 'StEpdGeom' which
has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
I'm not sure what to do with this...

I think other comments are implemented, besides having genFit track, which I leave it to Daniel and S&C meeting discussions.

plexoos · 2022-09-23T15:37:15Z

StFcsTrackMatchMaker::Finish(){
if(mEpdgeo) delete mEpdgeo;
causes
warning: deleting object of polymorphic class type 'StEpdGeom' which
has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
I'm not sure what to do with this...

How do you use the private member mEpdgeo in your code? All I can see is that you allocate an object and then delete it.

perevbnlgov · 2022-09-23T17:57:31Z

Speed is important. But speed gain from the suggested code seems minimal,

Akio, it depends. If number of candidates abbot 3 then 3*3=9, than gain is not big. If number is about 20, then 20*20=400 and gin is very big. You should account, that 80% of cases will dropped on one line of code. The test of 15% dropped after the second line line. So gain is big. If number about of 50+ then it is better to use for instance Hungarian algorithm

... and modified code seems bad to me.

Are you kidding?

But I need dR for debug and histograms...

In the case of debug, speed is not important. and in this case is possible to recalculate any variables what you need. Code should work fast only for production, not for debug.

... Please suggest other solution if you agree with me that the modified code looks bad.

If my code is bad for you, how do I know that the new code will not be bad for you again. Please explain why it is bad for you. But anyway, you, as an author of code has right to accept of not any proposal, because it is your responsibility. Victor

…

On 2022-09-23 10:37, akioogawa wrote: > (On dr calc in StFcsTrackMatchMaker.cxx) I do not know how speed is > important there. If it is important then it could be optimized. Speed is important. But speed gain from the suggested code seems minimal, and modified code seems bad to me. But I need dR for debug and histograms... Please suggest other solution if you agree with me that the modified code looks bad. StFcsTrackMatchMaker::Finish(){ if(mEpdgeo) delete mEpdgeo; causes warning: deleting object of polymorphic class type 'StEpdGeom' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor] I'm not sure what to do with this... I think other comments are implemented, besides having gent track, which I leave it to Daniel and S&C meeting discussions. -- Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. You are receiving this because you were mentioned.Message ID: ***@***.***> Links: ------ [1] #398 (comment) [2] https://github.com/notifications/unsubscribe-auth/ANQUL7O5DEHO3ZIHC5JHOP3V7W6BLANCNFSM6AAAAAAQLX3U4M

akioogawa · 2022-09-23T18:24:40Z

Speed is important. But speed gain from the suggested code seems minimal,
... and modified code seems bad to me.
Are you kidding?

No. But it is NOT your proposed code which looks bad to me. It is my modified code which looks bad to me.

If my code is bad for you

No. Your code looks ok without debugging lines. It is my code with debug & histogram filling which looks bad.

Please have a look at new commit and tell me if it looks ok for you.

akioogawa · 2022-09-23T18:39:14Z

Which is better?

  if(Debug()){
    double dx = xyz.x() - proj[ehp]->x();
    double dy = xyz.y() - proj[ehp]->y();
    double dr = sqrt(dx*dx + dy*dy);
    if(Debug()) LOG_INFO << Form("EHP=%1d dx = %6.2f - %6.2f  = %6.2f dy = %6.2f - %6.2f  = %6.2f dr=%6.2f",
				 ehp,xyz.x(),proj[ehp]->x(),dx,xyz.y(),proj[ehp]->y(),dy,dr) << endm;
  }
  if(mFile){
    double dx = xyz.x() - proj[ehp]->x();
    double dy = xyz.y() - proj[ehp]->y();
    double dr = sqrt(dx*dx + dy*dy);	    
    if(fabs(dy)<mMaxDistance[ehp]) mHdx[ehp]->Fill(dx);
    if(fabs(dx)<mMaxDistance[ehp]) mHdy[ehp]->Fill(dy);
    mHdr[ehp]->Fill(dr);
  }
  double dx = xyz.x() - proj[ehp]->x();  if(dx > mMaxDistance[ehp]) continue;
  double dy = xyz.y() - proj[ehp]->y();  if(dy > mMaxDistance[ehp]) continue;
  double dr = sqrt(dx*dx + dy*dy); if(dr > mMaxDistance[ehp]) continue;
  if(ehp==0){trk->addEcalCluster(clu);}
  else      {trk->addHcalCluster(clu);}

Or

      double dx = xyz.x() - proj[ehp]->x();
      double dy=0,dr=mMaxDistance[ehp];
      if(Debug() || mFile || dx < mMaxDistance[ehp]){
        dy = xyz.y() - proj[ehp]->y();
        if(Debug() || mFile || dy < mMaxDistance[ehp]){
            dr= sqrt(dx*dx + dy*dy);
        }
      }
      if(Debug()){
        if(Debug()) LOG_INFO << Form("EHP=%1d dx = %6.2f - %6.2f  = %6.2f dy = %6.2f - %6.2f  = %6.2f dr=%6.2f",
                                     ehp,xyz.x(),proj[ehp]->x(),dx,xyz.y(),proj[ehp]->y(),dy,dr) << endm;
      }
      if(mFile){
        if(fabs(dy)<mMaxDistance[ehp]) mHdx[ehp]->Fill(dx);
        if(fabs(dx)<mMaxDistance[ehp]) mHdy[ehp]->Fill(dy);
        mHdr[ehp]->Fill(dr);
      }
      if(dr < mMaxDistance[ehp]){
        if(ehp==0){trk->addEcalCluster(clu);}
        else      {trk->addHcalCluster(clu);}
     }

Former is commit this morning. Later Is still in my private file...

zlchang · 2022-10-21T17:43:43Z

StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.cxx

+	  double dx = xyz.x() - proj[ehp]->x();  if(dx > mMaxDistance[ehp]) continue;
+	  double dy = xyz.y() - proj[ehp]->y();  if(dy > mMaxDistance[ehp]) continue;


Do we need these two lines? The third line "dr" cut will automatically satisfy these cuts. If so, do we need to calculate dx = TMath::Abs(dx) and dy = TMath::Abs(dy) before comparing to the max distance?

perevbnlgov · 2022-10-21T20:14:27Z

Do we need these two lines? The third line "dr" cut will automatically

Yes, mathematically cut with dr only is equivalent with dx,dy,dr together. But timing is different. Let say %90 rejected in dx test. No dy and dr is needed to test The same with dy test. So practically time spend only for dx test, which is faster. Actually dr test could be omitted, there is no big differency between square (dx,dy) and round (dr). And also, if you want to test dr it is better to test dx*dx+dy*dy < mMaxDistance*mMaxDistance to avoid redundant calculation of sqrt.

and dy = TMath::Abs(dy) before comparing to the max distance?

Yes, you are right, it MUST be fabs(dx) Victor

…

satisfy these cuts. If so, do we need to calculate dx = TMath::Abs(dx) and dy = TMath::Abs(dy) before comparing to the max distance? and dy = TMath::Abs(dy) before comparing to the max distance? On 2022-10-21 13:43, Zilong Chang wrote: @zlchang commented on this pull request. ------------------------- In StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.cxx [1]: > + double dx = xyz.x() - proj[ehp]->x(); if(dx > mMaxDistance[ehp]) continue; + double dy = xyz.y() - proj[ehp]->y(); if(dy > mMaxDistance[ehp]) continue; Do we need these two lines? The third line "dr" cut will automatically satisfy these cuts. If so, do we need to calculate dx = TMath::Abs(dx) and dy = TMath::Abs(dy) before comparing to the max distance? -- Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. You are receiving this because you were mentioned.Message ID: ***@***.***> Links: ------ [1] #398 (comment) [2] #398 (review) [3] https://github.com/notifications/unsubscribe-auth/ANQUL7PHGFUGP7KJ6BPNACDWELI5XANCNFSM6AAAAAAQLX3U4M

jdbrice · 2023-02-04T14:45:06Z

OK. Previously this branch depended #397 which used the StFwdTrack addition to StEvent with dependency on GenFit.
Now the branch has been overwritten with a new history, depending on the new #492 which adds StFwdTrack to StEvent without any external dependencies.

The changes to THIS code (StFcsTrackMatchMaker) are very minimal - only a few variable name changes and the use of a helper function to access track projections instead of accessing them directly from the underlying vector.

So based on the past review, this should be ready to merge as soon as its parent branch is merged in.

Squashed #398 and rebased onto main. The resolution of conflicts leaned towards the code present in the main branch. No significant changes left so, I think #398 can be closed

The resolution of conflicts leaned towards the code present in the main branch. No significant changes left so, I think #398 can be closed

jdbrice · 2023-09-27T15:06:37Z

Merged in as part of #492

jdbrice requested review from akioogawa, klendathu2k, genevb and plexoos as code owners September 13, 2022 19:31

starsdong requested a review from zlchang September 15, 2022 18:11

akioogawa requested a review from nigmatkulov as a code owner September 15, 2022 18:37

starsdong requested a review from perevbnlgov September 16, 2022 12:18

akioogawa requested review from sunxuhit, yezhenyu2003 and techuan-huang as code owners September 16, 2022 15:15

klendathu2k reviewed Sep 22, 2022

View reviewed changes

zlchang reviewed Sep 22, 2022

View reviewed changes

StRoot/StEvent/StFwdTrack.cxx Show resolved Hide resolved

zlchang reviewed Sep 22, 2022

View reviewed changes

StRoot/StEvent/StFwdTrack.cxx Show resolved Hide resolved

plexoos mentioned this pull request Oct 11, 2022

BigFullChain, updates for FWD detectors and Tracking #396

Closed

zlchang reviewed Oct 21, 2022

View reviewed changes

zlchang approved these changes Oct 21, 2022

View reviewed changes

plexoos requested a review from veprbl as a code owner January 16, 2023 01:41

jdbrice added 15 commits February 2, 2023 08:55

initial addition of StFwdTrack to StEvent

6dadc2b

Add option to used mapped STAR Bfield, significant tracking speedup

7503308

Update FwdGeomUtils for new sTGC geom layout

e7ccf0b

Fix printf format warnings

971b312

Add FwdTrack ObjExporter for projections and visulaization options

7dadebc

access method to obtain TrackId from hit

cc246e5

StFwdTrack method to set primary momentum

9f1cf6a

Updates to track fitter for zero field, add GBL refit for alignment

ef65d36

Organize GenFit track results to avoid mismatch issues

4f1b1d8

Organize FwdTree TTree data structure

8228640

Update to StFwdTrackMaker to write to StEvent and debug output to TTree

ee1da70

Add general use macros for testing StFwdTrackMaker

fe77276

Fix StFwdTrackProjections helper function, missing {}

a64433a

fix missing HCAL projection

9d8e235

cleanup log messages

124e4c9

jdbrice force-pushed the fcs-track-match branch from 46b594b to 124e4c9 Compare February 4, 2023 14:40

plexoos mentioned this pull request Aug 23, 2023

Squashed commit of #398 #580

Closed

plexoos pushed a commit that referenced this pull request Aug 23, 2023

Squashed commit of #398

9d1e133

Squashed #398 and rebased onto main. The resolution of conflicts leaned towards the code present in the main branch. No significant changes left so, I think #398 can be closed

plexoos added a commit that referenced this pull request Aug 23, 2023

Squashed #398 and rebased onto main

763b6da

The resolution of conflicts leaned towards the code present in the main branch. No significant changes left so, I think #398 can be closed

plexoos mentioned this pull request Aug 23, 2023

Squashed #398 and rebased onto main #581

Closed

jdbrice closed this Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fwd Track matching to FCS clusters #398

Fwd Track matching to FCS clusters #398

jdbrice commented Sep 13, 2022 •

edited

akioogawa commented Sep 14, 2022 •

edited

plexoos commented Sep 14, 2022

jdbrice commented Sep 15, 2022

plexoos commented Sep 15, 2022

starsdong commented Sep 15, 2022

starsdong commented Sep 15, 2022

starsdong commented Sep 16, 2022

akioogawa commented Sep 21, 2022 •

edited

plexoos commented Sep 21, 2022

perevbnlgov commented Sep 22, 2022

klendathu2k commented Sep 22, 2022

klendathu2k Sep 22, 2022

jdbrice Oct 11, 2022

klendathu2k Oct 11, 2022

plexoos Oct 11, 2022

klendathu2k Sep 22, 2022

klendathu2k Sep 22, 2022

zlchang Sep 22, 2022

zlchang Sep 22, 2022

akioogawa commented Sep 23, 2022 •

edited

plexoos commented Sep 23, 2022

perevbnlgov commented Sep 23, 2022 via email

akioogawa commented Sep 23, 2022

akioogawa commented Sep 23, 2022 •

edited

zlchang Oct 21, 2022

perevbnlgov commented Oct 21, 2022 via email

jdbrice commented Feb 4, 2023

jdbrice commented Sep 27, 2023

	std::sort(mTracks.begin(), mTracks.end(), [](StFwdTrack* a, StFwdTrack* b) {
	std::sort(mTracks.begin(), mTracks.end(), [](const StFwdTrack* a, const StFwdTrack* b) {

		double dx = xyz.x() - proj[ehp]->x(); if(dx > mMaxDistance[ehp]) continue;
		double dy = xyz.y() - proj[ehp]->y(); if(dy > mMaxDistance[ehp]) continue;

Fwd Track matching to FCS clusters #398

Fwd Track matching to FCS clusters #398

Conversation

jdbrice commented Sep 13, 2022 • edited

akioogawa commented Sep 14, 2022 • edited

plexoos commented Sep 14, 2022

jdbrice commented Sep 15, 2022

plexoos commented Sep 15, 2022

starsdong commented Sep 15, 2022

starsdong commented Sep 15, 2022

starsdong commented Sep 16, 2022

akioogawa commented Sep 21, 2022 • edited

plexoos commented Sep 21, 2022

perevbnlgov commented Sep 22, 2022

klendathu2k commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akioogawa commented Sep 23, 2022 • edited

plexoos commented Sep 23, 2022

perevbnlgov commented Sep 23, 2022 via email

akioogawa commented Sep 23, 2022

akioogawa commented Sep 23, 2022 • edited

Choose a reason for hiding this comment

perevbnlgov commented Oct 21, 2022 via email

jdbrice commented Feb 4, 2023

jdbrice commented Sep 27, 2023

jdbrice commented Sep 13, 2022 •

edited

akioogawa commented Sep 14, 2022 •

edited

akioogawa commented Sep 21, 2022 •

edited

akioogawa commented Sep 23, 2022 •

edited

akioogawa commented Sep 23, 2022 •

edited