Skip to content

Conversation

@baltzell
Copy link
Collaborator

@baltzell baltzell commented Jul 1, 2025

This is #1101 plus #1114.

@baltzell baltzell changed the title Lcsim 4.5.0 fixtrackstates lcsim 4.5.0 plus fixtrackstates Jul 1, 2025
@baltzell baltzell mentioned this pull request Jul 1, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the lcsim dependency to 4.5.0, introduces an option to pivot track states at their plane intercept, and refactors Kalman interface methods for configurable pivot handling.

  • Upgrade lcsim version and add GitHub Maven repository
  • Add pivotAtIntercept flag through propagation and track-state creation
  • Clean up and extend Kalman/HPS bridging methods (e.g., new covariance extractor, coordinate transforms)

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tracking/.../PropagatedTrackState.java Pass new pivotAtIntercept flag into toTrackState call
tracking/.../KalmanInterface.java Add pivotAtIntercept parameter, reorganize toHPShelix/toTrackState, new helpers
tracking/.../KalTrack.java Implement getHelixAtPlane(...) with RK propagation
tracking/.../HelixState.java Overload RK propagation to accept pivotAtIntersect
tracking/.../KFOutputDriver.java Disable default residual/raw‐hit plots, add large‐2D plots toggle
recon/.../HpsReconParticleDriver.java Switch vertexer to use perigee field, update IP→perigee state indexing
pom.xml Bump <lcsimVersion> to 4.5.0 and add GitHub Packages repo
.github/workflows/test.yml Move from Java 1.8 to JDK 21, configure GITHUB_TOKEN
.github/dependabot.yml Add daily Dependabot updates for Maven and GitHub Actions
Comments suppressed due to low confidence (2)

tracking/src/main/java/org/hps/recon/tracking/kalman/KalmanInterface.java:391

  • [nitpick] Inline author/comment tags and dates may be better captured in commit history; remove dated comments or convert to descriptive Javadoc.
    // 5/10/25:  change variable name from alphaCenter to alpha to avoid confusion -- mgraham    

Comment on lines +729 to +735
System.out.println(this.getClass().getName()+"::getHelixAtPlane: Couldn't find a site???");
}else{
if(debug){
System.out.println(this.getClass().getName()+"::getHelixAtPlane:");
closeSite.print("closest measurement site");
}
}
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

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

If closeSite is null, the method proceeds and will dereference closeSite, causing an NPE. Add an early return or throw a clear exception when no site is found.

Suggested change
System.out.println(this.getClass().getName()+"::getHelixAtPlane: Couldn't find a site???");
}else{
if(debug){
System.out.println(this.getClass().getName()+"::getHelixAtPlane:");
closeSite.print("closest measurement site");
}
}
throw new IllegalStateException(this.getClass().getName() + "::getHelixAtPlane: Couldn't find a site.");
}
if(debug){
System.out.println(this.getClass().getName()+"::getHelixAtPlane:");
closeSite.print("closest measurement site");
}

Copilot uses AI. Check for mistakes.
helixAtTarget.print("helix with pivot at final plane");
pln.X().print("origin of final field system in global system");
Rot.print("rotation matrix from global to final field system");
System.out.println("Return parameters with pivot at the intersection:: "+pivotAtIntersect);
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

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

Debug System.out.println calls remain in the RK propagator; replace with a proper logger or remove for production.

Suggested change
System.out.println("Return parameters with pivot at the intersection:: "+pivotAtIntersect);
logger.log(Level.FINE, "Return parameters with pivot at the intersection: {0}", pivotAtIntersect);

Copilot uses AI. Check for mistakes.
@baltzell
Copy link
Collaborator Author

baltzell commented Jul 1, 2025

The (new) separate build-only, no-test, CI job now passes:)

@mgignac
Copy link
Collaborator

mgignac commented Jul 2, 2025

It looks like the reference files need to be updated? Looking at the failed tests, the momentum/mass/etc are different (which is expected for these changes):
Error: Failures: Error: EngRun2015MollerReconTest>ReconTest.testRecon:380->ReconTest.comparePlots:252->ReconTest.compareTrees:272->ReconTest.compareHistograms:293 Bin 2 is different for: Moller Bottom Track Momentum expected:<2> but was:<0> Error: EngRun2015V0ReconTest>ReconTest.testRecon:380->ReconTest.comparePlots:252->ReconTest.compareTrees:272->ReconTest.compareHistograms:293 Bin 0 is different for: V0 Bottom Track Momentum expected:<45> but was:<29> Error: PhysRun2016MollerReconTest>ReconTest.testRecon:380->ReconTest.comparePlots:252->ReconTest.compareTrees:272->ReconTest.compareHistograms:293 Bin 37 is different for: Moller Bottom Track Momentum expected:<2> but was:<0> Error: PhysRun2016V0ReconTest>ReconTest.testRecon:380->ReconTest.comparePlots:252->ReconTest.compareTrees:272->ReconTest.compareHistograms:293 Bin 27 is different for: V0 Invariant Mass expected:<1> but was:<0>

@baltzell baltzell mentioned this pull request Jul 7, 2025
@baltzell
Copy link
Collaborator Author

baltzell commented Jul 9, 2025

Retriggered actions now manually to see if SLAC maven repos are back online ...

Copy link
Contributor

@bloodyyugo bloodyyugo left a comment

Choose a reason for hiding this comment

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

Finally!

@bloodyyugo bloodyyugo merged commit 3149520 into master Jul 15, 2025
2 of 3 checks passed
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.

4 participants