Skip to content

Conversation

@klendathu2k
Copy link
Contributor

@klendathu2k klendathu2k commented Feb 21, 2024

Draft PR for geant4star. Supersedes #617.

Branch was rebased against current main (02/21/24). Plus few additional fixes to get it to run (up to loading G3) in the current environment.

NOTE: The code should be functional, but requires two modules which we don't currently have in spack, plus an update to a newer version of the geant4 library. Specifically, we need to add a GEANT3 VMC library, and update the Geant 4 VMC library to one which supports multi-engine simulations.

These versions have been tested together by the VMC team.

I should also mention that we are at a point of divergence in the ROOT / VMC development. Currently we are using the VMC version 1.1, which is built with ROOT. The next version is 2.0, which is shipped separately. The GEANT3 VMC 3.9p1 release includes fixes to sensitive volumes which we require, and is the last version able to run with VMC 1.1. Any move to a new version of ROOT, VMC and/or Geant (either G3 or G4) likely entails advancing to a new version of all of these packages... with the possible exception of ROOT.

geant4star

Code compiles against root 6.24 in our production environment:

starver dev config/v0.3.0-rhel7-root6.24.06
setenv STAR .
cons
  • StGeant4Maker should be excluded from all ROOT5 builds (in CI)
  • StGeant4Maker should be excluded from 32 bit builds
  • root5 and root6 builds should pass CI tests

[g4star] Update stacker and agml extension classes to enable multi engine tracking and user defined hits in the geant4 application.  Few additional fixes and cleanups in the code.

[g4star] Import the StGeant4Maker into the repository.  Builds under root 6.24 (with some additional compilation flags).

[g4star] Defer mysql load until StBFChain can do it.

[g4star] Add geant4vmc, geant4mk and fastjet chain options.  Load libfastjet with stargen package b/c one or more filters depends on it.  (Should consider splitting filters into sep chain opt).

[g4star] Cleanup commented out includes.

[g4star] Reduce compile time warnings...

[g4star] ... more reduction of compiler warnings

... and even more reduction of compiler warnings...

[g4star] And modify cons to pickup the include paths to geant4 and geant4 vmc libraries
…e" option (which was meant as documentation).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AgMLBTofVolumeId and similarly named classes implement the unique volume IDs defined in pams/sim/g2t/g2t_volume_id.g. They provide the unique associated between hits and hardware channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AgStarDummy inserts a few dummy routines (mangled to look like FORtran routines) which satisfy dependencies between the StarGenerator package and starsim.


/// Return a pointer to the event
StarGenEvent *event() { return mPrimaryEvent; }
/// Return a pointer to the stack
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exposes the particle stack to StGeant4Maker for PushPrimaries.


}

std::map<TString, AgMLExtension*> AgMLExtension::mExtensionMap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GEANT3 VMC took use of both the "user" and "framework" extensions to TGeoVolume, leaving us with the only option of keeping a map between volume names and extension data.

@klendathu2k klendathu2k marked this pull request as ready for review February 21, 2024 18:23
Copy link
Member

@iraklic iraklic left a comment

Choose a reason for hiding this comment

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

does this mean we did not have btof and eemc in AgML format before? (I though we did)

@klendathu2k
Copy link
Contributor Author

does this mean we did not have btof and eemc in AgML format before? (I though we did)

I assume you are referring to the "volume ID" classes? No. These are new classes to support the geant4star application.

@plexoos
Copy link
Member

plexoos commented Mar 6, 2024

If you have a chance please try to build with this dependencies

module load geant4-vmc-5-0-p5-root-6.24.06 geant4-10.5.1 geant3-3-8-root-6.24.06

Geant3-3-9-p1 is currently missing from the spack repo so, I'll need to add it

@klendathu2k
Copy link
Contributor Author

klendathu2k commented Mar 6, 2024 via email

@plexoos
Copy link
Member

plexoos commented Mar 6, 2024

geant3@3-9-p1 is also available on the SDCC machines now. So, the following modules can be used for testing:

module load geant4-vmc-5-0-p5-root-6.24.06 geant4-10.5.1 geant3-3-9-p1-root-6.24.06

@klendathu2k
Copy link
Contributor Author

With the last three pushes, code compiles and runs the TPC to completion. Albeit with no hits generated. Reason TBC.

klendathu2k and others added 28 commits May 20, 2025 01:01
…t in events generated on each Make of the chain.
1. Geometry option will be looked for in the bfc, physicssim chains.
   Finally lookup via call to GetDatabase.
2. Under embed mode, responsability for setting the vertex will be
   under the control of an embedding maker.
@klendathu2k klendathu2k requested a review from Copilot November 18, 2025 13:42
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 imports the geant4maker codebase to enable Geant4-based simulations within the STAR framework. The changes primarily involve removing obsolete StarVMC application files and test utilities, updating geometry configurations, adding support for Root6/Cling, and introducing Geant4-specific simulation infrastructure.

Key Changes

  • Removal of obsolete StarVMC application implementation files and associated testing infrastructure (StarBASE)
  • Updates to AgML geometry handling and extensions to support both Geant3 and Geant4 engines
  • Root6/Cling compatibility improvements across multiple macro and header files
  • Addition of embedding support macros and unit testing framework for Geant4

Reviewed Changes

Copilot reviewed 143 out of 1373 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
StarVMC/StarVMCApplication/* Removed obsolete VMC application headers and implementations
StarVMC/StarBASE/* Removed baseline analysis infrastructure no longer needed
StarVMC/StarAgmlLib/* Updated AgML extensions to support multi-engine simulations
StRoot/macros/*.C Added Root6/Cling preprocessor directives for compatibility
StRoot/StarGenerator/* Enhanced generator framework with improved maker registration
StRoot/St_geant_Maker/* Added documentation and embedding preparation infrastructure
StarVMC/Geometry/StgmGeo/StgmGeo1.xml Updated geometry to separate sensitive volume materials

TString GetVolumeName(){ return mVolumeName; }

int GetVolumeId( int* numbv ){ return mVolumeId ? mVolumeId->id( numbv ) : 0; }
int GetVolumeId( int* numbv ){ return mVolumeId->id( numbv ); }
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Potential null pointer dereference. The previous destructor removed the delete mVolumeId and nulled the pointer, but this getter does not check if mVolumeId is null before dereferencing it. This could cause a crash if GetVolumeId is called after the object is destroyed or before SetVolumeIdentifier is called.

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +37
void AgMLExtension::Print( Option_t* opts ) const {

//static const char* en[] = { "default", "geant3", "geant4" };

LOG_INFO << mModuleName.Data() << ":"
<< mFamilyName.Data() << ":"
<< mVolumeName.Data() << " this="
<< this << " sens="
<< mSensitive << " tracking="
<< mTracking << " nbranch="
<< mBranchings << " nuser="
// << mHitScoring.size() << " engine="
// << (mEngine>=0 && mEngine < 2) ? en[mEngine] : "invalid!? "
<< endm;

}
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Commented-out code should be removed rather than left in the source. Lines 24, 33, and 34 contain commented debug code that appears to be unfinished or unnecessary. Remove these lines to improve code clarity.

Copilot uses AI. Check for mistakes.

#if !defined(__CINT__) || defined(__CLING__)

#if !defined(__CINT__) || !defined(__CLING__)
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Incorrect conditional logic. The condition !defined(__CINT__) || !defined(__CLING__) will always evaluate to true (unless both CINT and CLING are defined, which is unlikely). This should use && instead: #if !defined(__CINT__) && !defined(__CLING__)

Suggested change
#if !defined(__CINT__) || !defined(__CLING__)
#if !defined(__CINT__) && !defined(__CLING__)

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +12
#ifndef __CLING__
#include "TClonesArray.h"
#endif
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Inconsistent use of preprocessor guards. The file uses #ifndef __CLING__ on line 10 but #ifndef __CINT__ is used elsewhere in the codebase. Consider using a consistent pattern across all files, preferably #if !(defined(__CINT__) || defined(__CLING__)) as seen in other files.

Copilot uses AI. Check for mistakes.
AgMLScoring* scoring = kv.second;
agmlExt -> AddHitScoring( scoring );
}
// NOTE: user hits have not been defined at this point, add to extension at point of definition
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

[nitpick] While this comment explains why user hits are not added here, it would be helpful to reference where in the code they are actually added (AgModule.cxx line 95-107) for better traceability.

Suggested change
// NOTE: user hits have not been defined at this point, add to extension at point of definition
// NOTE: user hits have not been defined at this point; they are added to the extension at the point of definition (see AgModule.cxx lines 95-107 for details)

Copilot uses AI. Check for mistakes.
agmlExt->SetFamilyName( block->GetName() );
agmlExt->SetModuleName( module->GetName() );
agmlExt->SetSensitive( mMedium.par("isvol") );
agmlExt->SetModuleName( module->GetName() );
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

[nitpick] The code now checks both medium and material for the 'isvol' parameter. Consider adding a comment explaining this fallback logic and why checking both is necessary for proper sensitive volume detection.

Suggested change
agmlExt->SetModuleName( module->GetName() );
agmlExt->SetModuleName( module->GetName() );
// Check both medium and material for the 'isvol' parameter.
// The sensitive volume flag ('isvol') may be defined in either the medium or the material.
// We first check the medium, and if not set, fall back to the material to ensure proper sensitive volume detection.

Copilot uses AI. Check for mistakes.
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