Skip to content

Conversation

@jdbrice
Copy link
Contributor

@jdbrice jdbrice commented Jan 17, 2024

Adds initial PicoDst integration for Forward systems:

  • StPicoFwdTrack
  • StPicoFwdVertex
  • StPicoFcsHit
  • StPicoFcsCluster

Details:

  • SPicoFwdTrack is a mirror of the StEvent/StMuDst version WITHOUT storing the hits associated with each track, and without keeping extra info about Genfit object
  • StPicoFwdVertex: These are secondary (or potentially primary when running on data without TPC) vertices found by the FWD tracker. They are stored in the StVertex collection at the StEvent/StMuDst level and only stored separately for the PicoDst if they pass certain quality criteria on their tracks constituents. These are mostly for e.g. Lambda analysis
  • StPicoFcsHit/Cluster - slimmed down versions of StEvent/MuDst equivalents. The hits are required for jet studies.

Size comparisons:

Vanilla: 12.8 Kb / Event
+FCS: 17.1 Kb / Event
+Fwd Tracks: 17.8 Kb / Event

@genevb
Copy link
Contributor

genevb commented Jan 19, 2024

From the discussion at this week's analysis meeting, it sounds like we should freeze an SL24a library shortly after this code is merged, perhaps after a day of testing it in DEV, as there is interest in using it in the ongoing preview production of Run 22 data (which is already resuming without this).

@jdbrice jdbrice added the FWD Forward Upgrade label Aug 22, 2024
Copy link
Contributor

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 adds initial PicoDst integration for Forward systems, introducing four new classes: StPicoFwdTrack, StPicoFwdVertex, StPicoFcsHit, and StPicoFcsCluster. The implementation enables storage of forward tracking and calorimetry data in a compressed format suitable for physics analyses.

Key changes:

  • Adds new PicoDst classes for forward detector systems (FST/FTT tracking, FCS calorimetry)
  • Integrates forward track and vertex storage with existing PicoDst infrastructure
  • Implements a "vtxless" mode for forward-only events without TPC primary vertices

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
StPicoFwdTrack.h/cxx New forward track class with momentum, fit quality, and calorimeter matching
StPicoFwdVertex.h New forward vertex class storing position and track association
StPicoFcsHit.h/cxx New FCS hit class with four-momentum storage
StPicoFcsCluster.h/cxx New FCS cluster class with shape and energy information
StPicoDstMaker.cxx/h Integration of forward objects and vtxless mode support
StPicoArrays.h/cxx Addition of new array types and sizing
StPicoDst.h/cxx Interface methods for accessing forward objects

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@genevb
Copy link
Contributor

genevb commented Aug 25, 2025

General comment:
If we're worried about memory build-up generated by LOG_DEBUG lines, then please consider using the following inside classes that inherit from StMaker:

if (debug()) { LOG_INFO << blah << endm; }

or LOG_WARN as you see fit.
(NB: the curly brackets around LOG_* messages after an if are necessary because the LOG_* are actually macros that resolve to multiple lines of code.)

@jdbrice
Copy link
Contributor Author

jdbrice commented Aug 28, 2025

Summary (Aug 28 12pm):

  • One "breaking" change was reverted - dont return early from StPicoDstMaker::fillTracks() if muDst->primaryTracks == nullptr. Instead skip only the primary track loop.
  • LOG_WARN on unmatched StEvent FCS clusters moved to once per event as LOG_ERROR
  • safety checks on pointers (non-null) before use were not removed. This is how the code should have been and prevents segfault / undefined behavior.
    All comments adequately addressed AFAIK

@jdbrice
Copy link
Contributor Author

jdbrice commented Aug 28, 2025

@jdb will check consistency of nightly test output to verify that the picots output is unchanged

Copy link
Member

@nigmatkulov nigmatkulov left a comment

Choose a reason for hiding this comment

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

Seems to be okay. @genevb will build the library for testing the changes.

@genevb
Copy link
Contributor

genevb commented Sep 8, 2025

I built an SL25z library (64-bit, on NFS4 only) with this PR, and ran several nightly tests for comparison with DEV (64-bit, AFS). Some of the tests I ran only produced MuDsts, so I did not have a closer look at them. The following nightly test datasets' PicoDsts were compared:

production_pp500_2022_64bit (optimized)
AuAu200_production_2016_64bit (unoptimized) (see footnote 1)
AuAu200_production_2016_64bit (optimized) (see footnote 1)
production_AuAu_2023_64bit (optimized)
production_AuAu_2024_64bit (optimized)
pp200_production_LowLuminosity_64bit (optimized)
27GeV_production_2018_64bit (unoptimized) (see footnote 1)
27GeV_production_2018_64bit (optimized) (see footnote 1)

I found in all cases (see footnote 2), the PicoDsts were essentially identical. This evidence supports moving forward with this PR.

  • Footnote 1: The jobs from years 2016 and 2018 (before the FWD subsystems existed) report these errors:

    StPicoDstMaker::ERROR - Cannot get StFcsDb object
    StPicoDstMaker::ERROR - null FwdTrackCollection

  • Footnote 2: The dev unoptimzed 27GeV saw a tiny difference in the number of primary tracks from the other three 27GeV jobs. Total numbers of global tracks were the same, however. So I'm inclined to believe this is a matter of rounding (least significant bits) differences, and not related to this PR.

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

The change I requested was made, so I'm changing my review status to "Approve".

@genevb
Copy link
Contributor

genevb commented Sep 9, 2025

...but it would be good if we can resolve the error messages. Perhaps a default DB entry for some table with beginTime = entryTime = "1996-01-01" would be appropriate?

@nigmatkulov
Copy link
Member

...but it would be good if we can resolve the error messages. Perhaps a default DB entry for some table with beginTime = entryTime = "1996-01-01" would be appropriate?

@jdbrice, could you please do it? It should be a simple change.

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

The suggestions I propose here deal with the unhelpful error messages for dataset which don't have FCS and/or FWD tracks. The understanding at today's S&C Management meeting was to make these changes for now to complete this PR so that we can proceed with freezing and building a library and move to a production.... but that it would be appropriate for a follow-up PR to more optimally handle these cases. There is code redundancy, and there should be no need for the fillFcs*() functions to be called at all if no FCS collection exists in the MuDst.

@jdb
Copy link

jdb commented Oct 3, 2025

@jdb will check consistency of nightly test output to verify that the picots output is unchanged

Just a note @jdb is not the right person, I don't participate to this repo. Probably @jdbrice is.

@genevb
Copy link
Contributor

genevb commented Oct 3, 2025

@jdb will check consistency of nightly test output to verify that the picots output is unchanged

Just a note @jdb is not the right person, I don't participate to this repo. Probably @jdbrice is.

Hahaha.... What makes that particularly funny is that @jdbrice is the one who wrote @jdb :-) Anyhow, sorry we bothered you! The authenticity of users included in these github discussions has been a concern of mine since the start :-/

@plexoos plexoos merged commit b26866e into star-bnl:main Oct 8, 2025
148 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FWD Forward Upgrade

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants