Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iss388 cvt cosmics #421

Open
wants to merge 12 commits into
base: development
Choose a base branch
from
Open

Iss388 cvt cosmics #421

wants to merge 12 commits into from

Conversation

zieglerv
Copy link
Collaborator

@zieglerv zieglerv commented Jan 8, 2025

No description provided.

ziegler added 10 commits December 12, 2024 10:41
…p to bottom.

In the case of a shallow angle ray there can be 2 intersection with a passive surface in the same hemisphere, hence the need to sort by y.
…he track if the next iteration chi^2 is worse.
… intersection point when there are 2 intersection points of the ray with the cylinder.
Use cluster information to determine y-intersection with the surface and hemisphere for active surfaces.
Compute the  y-intersection with the surface to be used for sorting in the kalman fitter.
@zieglerv zieglerv linked an issue Jan 8, 2025 that may be closed by this pull request
Copy link
Collaborator

@raffaelladevita raffaelladevita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks to me as if the main fix is in the calculation of the trajectory point that doesn't assume anymore the cylinders representing the detector surfaces are centered at 0,0, but I'm unsure about some of the changes to the measurement class

@@ -42,6 +42,7 @@ public class Surface implements Comparable<Surface> {
public double swimAccuracy;
public boolean passive = false;
public double hemisphere = 1;
public double rayYinterc = 9999;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make it inf or nan

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, to be more general, it could make sense to add a Point3D to store the full information of the trajectory point instead of only y

sv.z = inters.z();
sv.path = inters.distance(st);
}
// mv.surface.toLocal().apply(st);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove the commented-out lines...they will still be in the history

int ints = mv.surface.cylinder.intersection(toPln, inters);
if(ints<1) return false;

sv.x = inters.get(0).x() ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the first intersection always the right one?

this.setTrajectory(sv, mv);
setFitFailed = false;
//System.out.println(finalStateVec.toString());
//setFitFailed = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume setFitFailed is not really used elsewhere but it may be better to still set it to false
(and remove the commented text)

} else {
int index =0;
if(ints>1) {
double dh0 = Math.abs(inters.get(0).y()-mv.surface.rayYinterc);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the right intersection be the one with the largest y among the ones with y<rayInterc?

}
surfaces.add(surf);

sMap.put(surf.getIndex(), surf);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this changing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes are not really necessary

return hemisphereSign == (int) Math.signum(intersections.get(0).y());
case 2:
// Check if either of the two intersections match the hemisphere
return hemisphereSign == (int) Math.signum(intersections.get(0).y()) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for small y, isn't possible the sign doesn't match because of resolution?

if(surface.cylinder==null)
return 0;
List<Point3D> trajs = new ArrayList<>();
private double handleCylinderIntersection(int h, Cylindrical3D cylinder, Line3D line) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h could be renamed as hemisphere for clarity

double Y = this.getY(cluster, ray, surface);
if(Y == Double.POSITIVE_INFINITY) return 0;
surface.rayYinterc=Y;
return (int) Math.signum(Y);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if misalignments are large in y, the sign of the y coordinate of the intersection point doesn't really correspond to the hemisphere

ziegler added 2 commits January 9, 2025 19:48
Change isCrossed method to use the value of the hemisphere which is calculated 
as the sign of the y value of the intersection of the ray with the surface 
if it crosses it or 0 if it does not.
Fix intersection of ray with cylinder for tracks from the target.
Intersection initialized as positive infinity.
@@ -283,11 +297,14 @@ private void addClusters(DetectorType type, List<Surface> clusters) {
// }

private List<Surface> getClusterSurfaces(DetectorType type, List<Cluster> clusters, Ray ray) {

List<Surface> surfaces = this.getClusterSurfaces(type, clusters, ray);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems the initial source of the problem is this line, a call to the method from the same method. This was introduced with #351.

Fixing this and reverting to assigning the hemisphere based on the cluster.y instead of with the ray intersection seems to solve the problem.

The ray intersection with the surface doesn't seem to work consistently because when two intersection points are found (for cylindrical surfaces) the first one in the List is not necessarily the closest to the ray origin. This is the case also with intersectionRay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lots of log messages when reco CVT cosmic using 11.0.4
2 participants