JeffersonLab / coatjava

1 stars 18 forks source link

Fixes to DC geometry #299

Closed raffaelladevita closed 2 months ago

raffaelladevita commented 3 months ago

Accounts for:

For the latter only the shift to the wire midpoint is used while the change in direction is for now ignored because somehow tracking vertex resolution become worse when that is used. The current hypothsis is that is just a mathematical issue appearing because reconstruction is still assuming the stereo angle is 6 deg. The effect should be anyway negligible because the max change to the stereo angle is +/- 0.013 deg, corresponding to shifts of the wire endpoints of 10-30 um.

raffaelladevita commented 3 months ago

See this for more details

hauenst commented 3 months ago

I looked over the code and I have some comments and a open question which I already sent to Raffaella. I write them here for documentation purposes. In general we should also link later the new geometry document which we are going to write since drawings for most of the equations would be very helpful. All my comments are for DCGeant4Factory.java which has the main geometry changes 1) Line 600 - function 'public static MinistaggerStatus getStatus(String name)' Would need some comment that "ON" is used when "IF" condition fails. Also some print out warning 2) Line 610 - function 'public static MinistaggerStatus getStatus(boolean status)' - should have comment that this is for backup compatibility 3) Line 631 - function public static FeedthroughsStatus getStatus(String name) Would need some comment that "SHIFT" is used when "IF" condition fails. Also some print out warning 4) Line 666 Constructor 'public DCGeant4Factory(ConstantProvider provider, .... ' Need some printout in the logger on the configuration for ministagger, feedthrough and endplates --> easier for troubleshoot

More comments are all for function 'private void findEnds()' from line 356-415:

5) Comments for coordinate systems. It is confusing when some of the vectors are rotated by (theta_tilt) and when not. It would be great if comments could example and explicit state the systems. 6) Maybe the vectors rnorm,lnorm,lpar and rpar could be created at the same time to make reading easier 7) Scalar product line 373-374 and 375-376: Vectors of different coordinate systems are mixed (direction and #vnum/lnorm/rnorm). This should work but it would be good to check if the result is the same when all vectors are in the same coordinate system. Also comment should expand how the scalar product is used. 8) Line 399: comment for this vector is not fully true. It is a not the vector connection the points but just a vector of the correct length in the same direction with an arbitrary shift. Thats why we have to add rcirc in line 404 9) Line 401: langle is not used so the line should be removed 10) Line 400 and 403: I think these are the correct equations (besides the sign of the rotation, rotate function is doing it clockwise and I think we have required counterclockwise movement due to vperp pointing downstream) but it would be good to test it with an alternative angle and vector. We should arrive to the same result if we do: double angle_test = newDirection.angle(rpar.clone().rotateX(-dbref.thtilt(ireg)); //angle smaller than 90 degree vperp.rotate(rtang,90-angle_test); 11) Line 406: midpoint = rtang.plus(newDirection.times(-rtang.x/newDirection.x)); Question is this is general true for tilted and non-tilted frame.

raffaelladevita commented 2 months ago

The last four commits address the requested changes, details below.

1. Line 600 - function 'public static MinistaggerStatus getStatus(String name)' would need some comment that "ON" is used when "IF" condition fails. Also some print out warning Added printout warning which clarifies the behavior. 2. Line 610 - function 'public static MinistaggerStatus getStatus(boolean status)' - should have comment that this is for backup compatibility Added comment. 3. Line 631 - function public static FeedthroughsStatus getStatus(String name) would need some comment that "SHIFT" is used when "IF" condition fails. Also some print out warning Added printout warning which clarifies the behavior. 4. Line 666 Constructor 'public DCGeant4Factory(ConstantProvider provider, .... ' need some printout in the logger on the configuration for ministagger, feedthrough and endplates --> easier for troubleshoot Added printout

_5. Comments for coordinate systems. It is confusing when some of the vectors are rotated by (thetatilt) and when not. It would be great if comments could example and explicit state the systems. Done 6. Maybe the vectors rnorm,lnorm,lpar and rpar could be created at the same time to make reading easier Done 7. Scalar product line 373-374 and 375-376: Vectors of different coordinate systems are mixed (direction and #vnum/lnorm/rnorm). This should work but it would be good to check if the result is the same when all vectors are in the same coordinate system. Also comment should expand how the scalar product is used. The coordinate frame are not really different being always the sector frame. The comments have been expanded to specify this and explain the equations. Hopefully, this addresses the problem. 8. Line 399: comment for this vector is not fully true. It is a not the vector connection the points but just a vector of the correct length in the same direction with an arbitrary shift. Thats why we have to add rcirc in line 404 The comments have been corrected 9. Line 401: langle is not used so the line should be removed That is true but I would like to keep it for reference. A comment has been added to explain it. 10. Line 400 and 403: I think these are the correct equations (besides the sign of the rotation, rotate function is doing it clockwise and I think we have required counterclockwise movement due to vperp pointing downstream) but it would be good to test it with an alternative angle and vector. We should arrive to the same result if we do:

   double angle_test = newDirection.angle(rpar.clone().rotateX(-dbref.thtilt(ireg)); //angle smaller than 90 degree
   vperp.rotate(rtang,90-angle_test);

The test was done confermaing the resulting rtang coordinates are the same: [0.032871843756932584, 0.2828144097058451, -0.13187852508242096] [0.032871843756932445, 0.2828144097058451, -0.13187852508242096] The confusion on the sign of the rotation is due to a mistake in the comment defining perp: this vector in fact points upstream and not downstream as previously stated. 11. Line 406: midpoint = rtang.plus(newDirection.times(-rtang.x/newDirection.x)); Question is this is general true for tilted and non-tilted frame. The rationale for the equation above is that the midpoint is defined as the point along the wire that as x=0. In parametric representation, the points on the wire line are defined by: x = rtang.x +tnewDirection.x; y = rtang.y +tnewDirection.y; z = rtang.z +t*newDirection.z; The condition x=0 implies the value of the t parameter is t= - rtang.x/newDirection.x, hence the equation on line 406. This is independent on whether we are in the tilted frame or in the sector frame, since in both the midpoint is defined as the x=0 point.

baltzell commented 2 months ago

Let's goto 11? Even if it's technically backward-compatible, seems probably delicate.