Fwd Track matching to FCS clusters

[jdbrice]

Akio's Fwd track match maker for FCS clusters

Depends on https://github.com/star-bnl/star-sw/pull/396 and https://github.com/star-bnl/star-sw/pull/397

[akioogawa]

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]

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]

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]

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]

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]

Added Zilong Chang as a reviewer for the new maker.

[starsdong]

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]

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

[plexoos]

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

For some reason he does not want to join

[perevbnlgov]

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]

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: https://github.com/star-bnl/star-sw/pull/397#discussion_r972513178

[akioogawa]

(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]

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]

Speed is important. But speed gain from the suggested code seems minimal, Akio, it depends. If number of candidates abbot 3 then 33=9, than gain is not big. If number is about 20, then 2020=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] https://github.com/star-bnl/star-sw/pull/398#issuecomment-1256299297 [2] https://github.com/notifications/unsubscribe-auth/ANQUL7O5DEHO3ZIHC5JHOP3V7W6BLANCNFSM6AAAAAAQLX3U4M

[akioogawa]

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.

Yeah, not yet used. EPD-Track matching should be implemented in future, then we need that EPD geo... It is slightly more complicated since EPD-West is not x & y, but rather eta & phi and size is changing, so maxDistance will depends on eta. So I need a lot more study how to cut on it for less important info (EPD hit is already required at trigger level forDY/Jpsi trigger, and having track already indicate that it is charged track, EPD is just another, and less powerful, charged track ID). Yes I'll try to get to in soon, but will take time.

[akioogawa]

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]

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...

[perevbnlgov]

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 dxdx+dydy < 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] https://github.com/star-bnl/star-sw/pull/398#discussion_r1002042171 [2] https://github.com/star-bnl/star-sw/pull/398#pullrequestreview-1151336037 [3] https://github.com/notifications/unsubscribe-auth/ANQUL7PHGFUGP7KJ6BPNACDWELI5XANCNFSM6AAAAAAQLX3U4M

[jdbrice]

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.

[jdbrice]

Merged in as part of #492