Update clock jump correction, add new method for hit manipulation on counter level #457

YannickSoehngen · 2022-12-05T19:12:03Z

Added feedback method from Match to Hit-Maker as well as "double-jump" correction and method to modify Hits on counter level steered by Db table

…unter level

starsdong · 2022-12-06T18:55:55Z

Yannick, thanks for putting this PR out. It seems a few checks broke. Could you please help take some investigations on this? Thanks

YannickSoehngen · 2022-12-06T22:08:44Z

Sure! I forgot to set up a "default" entry in the data base wich is now in place and the checks finished on second attempt. December 6, 2022 7:56 PM, "Xin Dong" ***@***.*** ***@***.******@***.***>)> wrote: Yannick, thanks for putting this PR out. It seems a few checks broke. Could you please help take some investigations on this? Thanks — Reply to this email directly, view it on GitHub (#457 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AOLELK4E7TCI2BZSXZVL7KLWL6D4LANCNFSM6AAAAAASUUEFLM). You are receiving this because you authored the thread.Message ID: ***@***.***>

starsdong · 2022-12-06T22:16:47Z

Thank you, Yannick.
Philipp, would appreciate if you can review and approve this PR.
In the meantime, Yannick, I wonder whether you can do the MuDst->picoDst conversion test for Run21 7.7 GeV data and Run20 FXT data? This will be helpful for the readiness of reproduction.

starsdong · 2022-12-09T18:37:51Z

Philipp and Yannick, are we ready to merge? I would still like to see if you have tried a test run with the Run20 FXT data using this for MuDst->picoDst conversion and checked out the output?

StRoot/StETofHitMaker/StETofHitMaker.cxx

StRoot/StETofHitMaker/StETofHitMaker.h

StRoot/StETofHitMaker/StETofHitMaker.cxx

plexoos · 2022-12-12T03:29:02Z

StRoot/StETofHitMaker/StETofHitMaker.h


    std::map< Int_t, Int_t >     mClockJumpDirection;  // stores direction of clock jump for time correction

+    std::map< Int_t, int >     mModMatrix;  // stores mode of modification for hits on striplevel (flip)


The modes should be represented using an enum

I agree that magic numbers aren't optimal here but I would like to keep the integers as they will allow me to increase the granularity of that method to Get4 level (if need be).

I don't see how an enum would constrain the granularity... You currently have 5 modes modifying the hit coordinates.

Not as the method currently is. But If I'll have to increase it I can stick with our "default integer" naming scheme and wont have to introduce jet another db-table. I dont think those 5 modes will cause too much confusion, but retaining the flexibility here will allow for more easy and consistent solutions in the future.

StRoot/StETofUtil/etofHelperFunctions.cxx

StRoot/StETofUtil/etofHelperFunctions.h

starsdong · 2022-12-19T17:06:43Z

Hi Yannick, thank you for addressing the comments.
Currently the failed jobs seem to be related to the StrangeMuDst. Dmitri and Gene, I see the same issue with the DEV library has been addressed by setting back in the main branch. Is it safe to merge this PR now? Thanks

plexoos

There are still warnings in StETofHitMaker, please address

plexoos · 2022-12-20T20:54:21Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

+  if(modMode == 0) {return;}
+
+  if(modMode == 1){    
+    localX *= -1;
+    localY *= -1;
+  }
+  if(modMode == 2){
+
+    localX *= -1;
+  }
+  if(modMode == 3){    
+    localY *= -1;
+  }
+  if(modMode == 4){
+    double x = localX;
+    double y = localY;
+    localX =    y;
+    localY = -1*x;
+  }
+  if(modMode == 5){
+    double x = localX;
+    double y = localY;
+    localX = -1*y;
+    localY =    x;
+  }
+  if(modMode == 99){
+    localX = 9999;
+    localY = 9999; 
+  }
+


The above suggestions are reasonable. Is there any reason not to take them?

StRoot/StETofMatchMaker/StETofMatchMaker.cxx

StRoot/StETofUtil/etofHelperFunctions.cxx

StRoot/StETofHitMaker/StETofHitMaker.h

StRoot/StETofUtil/etofHelperFunctions.cxx

StRoot/StETofHitMaker/StETofHitMaker.cxx

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

plexoos

There are warnings in StETofHitMaker, please address

StRoot/StETofUtil/etofHelperFunctions.cxx

StRoot/StETofMatchMaker/StETofMatchMaker.cxx

StRoot/StETofHitMaker/StETofHitMaker.cxx

StRoot/StETofMatchMaker/StETofMatchMaker.cxx

StRoot/StETofUtil/etofHelperFunctions.cxx

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

StRoot/StETofHitMaker/StETofHitMaker.h

StRoot/StETofMatchMaker/StETofMatchMaker.cxx

StRoot/StETofMatchMaker/StETofMatchMaker.h

plexoos · 2023-01-03T18:13:42Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

+	const double safetyMargins[ 2 ] = { 0., 0. };
+        mETofGeom->init( gGeoManager, safetyMargins, 0 ); //don't use helix swimmer here. Probably needs an additional include


Suggested change

const double safetyMargins[ 2 ] = { 0., 0. };

mETofGeom->init( gGeoManager, safetyMargins, 0 ); //don't use helix swimmer here. Probably needs an additional include

mETofGeom->init( gGeoManager);

Looks like you pass default parameters, see

star-sw/StRoot/StETofUtil/StETofGeometry.cxx

Lines 566 to 568 in 81d6a97

void

StETofGeometry::init( TGeoManager* geoManager, const double* safetyMargins = initializer_list<double>({0, 0}).begin(), const bool& useHelixSwimmer = false )

{

StRoot/StETofHitMaker/StETofHitMaker.cxx

StRoot/StETofMatchMaker/StETofMatchMaker.h

plexoos · 2023-01-03T19:24:42Z

StRoot/StETofMatchMaker/StETofMatchMaker.cxx

+
+    // --------------------------------------------------------------------------------------------
+    // pointer to eTOF hit maker
+    // --------------------------------------------------------------------------------------------
+
+    mETofHitMaker = ( StETofHitMaker* ) GetMaker( "etofHit" );
+    LOG_DEBUG << "StETofMatchMaker::InitRun() -- pointer to eTOF hit maker: " << mETofHitMaker << endm;
+


No maker should directly access another maker. This would entangle the dependency between makers.

plexoos · 2023-01-04T23:09:20Z

I already asked multiple times (see this thread) to fix the warnings produced by the modified code... Well now it is an error, cannot proceed before it is fixed

#10 831.4 .sl79_gcc485/OBJ/StRoot/StETofHitMaker/StETofHitMaker.cxx: In member function 'void StETofHitMaker::fillHitQA(bool, const double&)':
#10 831.4 .sl79_gcc485/OBJ/StRoot/StETofHitMaker/StETofHitMaker.cxx:1878:40: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]
#10 831.4              if( fabs( tstart ) > 0.001 && fabs( tstart - ( eTofConst::bTofClockCycle - 9999. ) ) > 0.001 || mIsSim ) {
#10 831.4                                         ^
#10 831.4 .sl79_gcc485/OBJ/StRoot/StETofHitMaker/StETofHitMaker.cxx:1924:49: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]
#10 831.4              if( mDoQA && fabs( tstart ) > 0.001 && fabs( tstart - ( eTofConst::bTofClockCycle - 9999. ) ) > 0.001 || mIsSim ) {
#10 831.4                                                  ^
#10 831.4 .sl79_gcc485/OBJ/StRoot/StETofHitMaker/StETofHitMaker.cxx:2022:40: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]
#10 831.4              if( fabs( tstart ) > 0.001 && fabs( tstart - ( eTofConst::bTofClockCycle - 9999. ) ) > 0.001 || mIsSim ) {
#10 831.4                                         ^
#10 831.4 .sl79_gcc485/OBJ/StRoot/StETofHitMaker/StETofHitMaker.cxx:2064:49: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]
#10 831.4              if( mDoQA && fabs( tstart ) > 0.001 && fabs( tstart - ( eTofConst::bTofClockCycle - 9999. ) ) > 0.001 || mIsSim ) {
#10 831.4                                                  ^
#10 831.9 cc1plus: all warnings being treated as errors

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

plexoos · 2023-01-04T23:17:51Z

StRoot/StETofHitMaker/StETofHitMaker.h


+    void setGet4MinTime( const double& minTime );


This is not how I suggested to fix the signature of this function. Adding 'const' is fine but passing a double by reference feels weird and superfluous

plexoos · 2023-01-06T04:42:18Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

-        mETofGeom->init( gGeoManager); //don't use helix swimmer here. Probably needs an additional include
-	LOG_DEBUG << " init done " << endm;
+	const double safetyMargins[ 2 ] = { 0., 0. };
+	mETofGeom->init( gGeoManager, safetyMargins, 0 ); //don't use helix swimmer here. Probably needs an additional include


Why did you decide to revert the changes as I suggested in #457 (comment)? What kind of bug does this fix?

It would be easier to understand your intentions and the problems you are trying to fix if you replied with some comments rather than just making changes

Sorry, 2 of the tets crashed here and I wanted to see if the other changes pass them. The suggested change caused a:
"symbol lookup error: /direct/star+u/ysoehngen/Work/TestHitnew/.sl73_gcc485/lib/libStETofHitMaker.so: undefined symbol: _ZN14StETofGeometry4initEP11TGeoManager"

plexoos · 2023-01-06T04:45:46Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

 		     StMuPrimaryVertex* pVtx = mMuDst->primaryVertex( 0 );
-		     StThreeVectorD posGlo = mETofGeom->hitLocal2Master( constructedHit );
-		     
+		     StThreeVectorD*  posGlo = new StThreeVectorD( mETofGeom->hitLocal2Master( constructedHit ));
+
 		     if( pVtx ) {
-		       StThreeVectorD vtxPos = pVtx->position();		
+		       StThreeVectorD* vtxPos = new StThreeVectorD( pVtx->position());	
 		       exptof =  tofPathLength(vtxPos , posGlo , 0) / ( nanosecond * c_light );


You just introduced a memory leak. This must be fixed. If a function takes a pointer, you can pass the address of a variable.

plexoos · 2023-01-06T04:48:17Z

StRoot/StETofMatchMaker/StETofMatchMaker.cxx

+		  std::string histName_t0corr_jump = "matchCand_t0corr_jump_s" + std::to_string( matchCand.sector ) + "m" + std::to_string( matchCand.plane ) + "c" + std::to_string( matchCand.counter );
+		  mHistograms.at( histName_t0corr_jump )->Fill( matchCand.localX, tof - tofpi ); //cutdownYS
+
+		  int get4Index = matchCand.sector * 1000 + matchCand.plane * 100 + matchCand.counter * 10 + ( matchCand.localX + 16 ) / 4 + 1;


Is the '16' in this expression the same as 'mLocalYmax'?

No, this shifts the local x coordinate (-16 to 16) to strip-numbers (32 strips in total)

plexoos · 2023-01-06T04:49:50Z

StRoot/StETofMatchMaker/StETofMatchMaker.h

+
+        Int_t           idTruth;
+        Int_t           idTruthHit; 
+	Int_t           nHitsFit;   
+	bool            isMissMatch; 


I don't see how these new members are used in this PR. Are they needed? If not, please remove, i.e. don't add

plexoos · 2023-01-06T04:52:58Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

+		     }
+
+		     if(mGet4doublejumpFlag.at(get4Nr) == 1){
+		       constructedHit->setTime(constructedHit->time() + 6.25);


This '6.25' is already defined as a named constant, isn't it?

star-sw/StRoot/StETofUtil/StETofConstants.h

Line 63 in 81d6a97

const double coarseClockCycle = 6.25; // [ns] -- needs to be double otherwise strange things happen ...

plexoos · 2023-01-06T04:56:08Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

+	    bool IsMissmatch = false; 
+	    int  IdTruth     = 0; 


According to STAR coding guidelines variables should start with a lower case letter: https://star-bnl.github.io/star-coding-guidelines/formatguide.xml#Variable_Names

plexoos · 2023-01-06T05:05:08Z

StRoot/StETofHitMaker/StETofHitMaker.cxx

+    x        = localX;
+    localX   =   localY;
+    localY   = -1*x;


How about this?

Suggested change

x = localX;

localX = localY;

localY = -1*x;

std::swap(localX, localY), localY = -localY;

This reverts commit 79f22f2.

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

starsdong · 2023-01-11T17:29:38Z

Dmitri, could you please take a (final?) look at the new commits by Yannick? If OK, I think we are ready to merge this PR to main.

plexoos · 2023-01-11T17:58:22Z

Xin, if you think it is ready just go ahead and approve. I am away this week and won't have a chance to look at it until later. From the thread it should be clear what's been addressed and what hasn't.

starsdong

Thanks everyone for helping on this. To me, it looks ready to be merged.

ClockJump_correction update and new method for Hit manipulation on co…

9a72c75

…unter level

YannickSoehngen requested a review from PhilippWeidenkaff as a code owner December 5, 2022 19:12

added missing etofHelperFunctions

9889653

PhilippWeidenkaff approved these changes Dec 12, 2022

View reviewed changes

plexoos reviewed Dec 12, 2022

View reviewed changes

Yannick Soehngen and others added 2 commits December 15, 2022 10:09

addressed suggested changes

ffa9088

Merge branch 'main' into ClockJumpCorrection-and-CounterMod-update

febf3f7

Merge branch 'main' into ClockJumpCorrection-and-CounterMod-update

c62c0a9

plexoos reviewed Dec 20, 2022

View reviewed changes

Yannick Soehngen added 2 commits December 21, 2022 09:00

2nd round of comments

8551432

Merge branch 'ClockJumpCorrection-and-CounterMod-update' of https://g…

0923a14

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

plexoos reviewed Dec 21, 2022

View reviewed changes

change white space

b9dcf39

plexoos reviewed Dec 21, 2022

View reviewed changes

StRoot/StETofUtil/etofHelperFunctions.cxx Outdated Show resolved Hide resolved

plexoos changed the title ~~ClockJump_correction update and new method for Hit manipulation on counter level~~ Update clock jump correction, add new method for hit manipulation on counter level Dec 22, 2022

Yannick Soehngen added 2 commits December 28, 2022 11:27

3d round of comments

7cd0372

Merge branch 'ClockJumpCorrection-and-CounterMod-update' of https://g…

fa5e2c5

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

plexoos reviewed Jan 3, 2023

View reviewed changes

Yannick Soehngen and others added 2 commits January 4, 2023 09:24

4th round of comments

9a8fd38

Merge branch 'main' into ClockJumpCorrection-and-CounterMod-update

a38037f

plexoos added this to the SL23a milestone Jan 4, 2023

Yannick Soehngen added 3 commits January 5, 2023 04:15

5th round of comments

eda3e89

Merge branch 'ClockJumpCorrection-and-CounterMod-update' of https://g…

e10c291

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

bug fix in HitMaker

79f22f2

plexoos reviewed Jan 6, 2023

View reviewed changes

plexoos and others added 8 commits January 6, 2023 11:19

Clean up white space according to coding guidelines

ef9e5b6

remove undefined class method

98f1101

Revert "bug fix in HitMaker"

7139608

This reverts commit 79f22f2.

Tweak StETofGeometry::init() default params

fa6331e

renamed variables and replaced 6.25 by etofConstant

4c77e07

Merge branch 'ClockJumpCorrection-and-CounterMod-update' of https://g…

591ab4e

…ithub.com/YannickSoehngen/star-sw into ClockJumpCorrection-and-CounterMod-update

removed unused variable

9e25945

adjusted default electronics dead-time

b9b1559

starsdong approved these changes Jan 12, 2023

View reviewed changes

starsdong merged commit ccd8dc0 into star-bnl:main Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update clock jump correction, add new method for hit manipulation on counter level #457

Update clock jump correction, add new method for hit manipulation on counter level #457

YannickSoehngen commented Dec 5, 2022

starsdong commented Dec 6, 2022

YannickSoehngen commented Dec 6, 2022 via email

starsdong commented Dec 6, 2022

starsdong commented Dec 9, 2022

plexoos Dec 12, 2022

YannickSoehngen Dec 21, 2022

plexoos Dec 21, 2022

YannickSoehngen Dec 29, 2022

starsdong commented Dec 19, 2022

plexoos left a comment

plexoos Dec 20, 2022

plexoos left a comment

plexoos Jan 3, 2023

plexoos Jan 3, 2023

plexoos commented Jan 4, 2023

plexoos Jan 4, 2023

plexoos Jan 6, 2023

YannickSoehngen Jan 9, 2023

plexoos Jan 6, 2023 •

edited

Loading

plexoos Jan 6, 2023

YannickSoehngen Jan 9, 2023

plexoos Jan 6, 2023

plexoos Jan 6, 2023

plexoos Jan 6, 2023

plexoos Jan 6, 2023 •

edited

Loading

starsdong commented Jan 11, 2023

plexoos commented Jan 11, 2023

starsdong left a comment


		std::map< Int_t, Int_t > mClockJumpDirection; // stores direction of clock jump for time correction

		std::map< Int_t, int > mModMatrix; // stores mode of modification for hits on striplevel (flip)

		const double safetyMargins[ 2 ] = { 0., 0. };
		mETofGeom->init( gGeoManager, safetyMargins, 0 ); //don't use helix swimmer here. Probably needs an additional include

	const double safetyMargins[ 2 ] = { 0., 0. };
	mETofGeom->init( gGeoManager, safetyMargins, 0 ); //don't use helix swimmer here. Probably needs an additional include
	mETofGeom->init( gGeoManager);

	void
	StETofGeometry::init( TGeoManager* geoManager, const double* safetyMargins = initializer_list<double>({0, 0}).begin(), const bool& useHelixSwimmer = false )
	{

Update clock jump correction, add new method for hit manipulation on counter level #457

Update clock jump correction, add new method for hit manipulation on counter level #457

Conversation

YannickSoehngen commented Dec 5, 2022

starsdong commented Dec 6, 2022

YannickSoehngen commented Dec 6, 2022 via email

starsdong commented Dec 6, 2022

starsdong commented Dec 9, 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

starsdong commented Dec 19, 2022

plexoos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plexoos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plexoos commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plexoos Jan 6, 2023 • edited Loading

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

plexoos Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

starsdong commented Jan 11, 2023

plexoos commented Jan 11, 2023

starsdong left a comment

Choose a reason for hiding this comment

plexoos Jan 6, 2023 •

edited

Loading

plexoos Jan 6, 2023 •

edited

Loading