Update StarGenerator and starsim codes in preparation for geant4star integration #103

klendathu2k · 2021-08-13T21:05:30Z

Changes to the StarGenerator and starsim codes in preparation for geant4star integration.

Introduce ageventread_() as a callback from starsim (agzio.age) triggering event generation. This allows existing workflows with St_geant_maker to remain untouched.
Goal was to remove the dependency between StarGenerator and the starsim codes. It will be difficult to eliminate the dependence on the agseventread_ (&etc...) methods in StarGenerator. We will need to supply these dummy symbols in geant4star at integration.
Introduce BFC chain options to load StarGenerator framework, and some select concrete generators
Several code cleanups identified in review
StarParticleData no longer created at library load (static instance of object vs dynamically instanstantiated)
"usestarsim" attribute added (default) for StarPrimaryMaker.
Introduced trivial test macro.

Point is well taken. Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>

…ncies

on the starsim libraries out of the event generator framework. The AgStarReader framework is moved into St_geant_Maker. Initialization of the reader presents a small problem. Most of our example macros have delayed initialization of the generator framework until *after* the initialization of the full chain. So we don't have a guarenteed hook to setup the connection between the event generator and the starsim package. Thus, the only place we can do this is on the first call to St_geant_Maker::Make. It is somewaht ugly, but the alternative is to change the initialization scheme, change all of the production and example macros, AND handle the fallout of breaking *users* macros.

…ing).

Should resolve the compiler error due to the moved file: StRoot/StarGenerator/BASE/AgStarReader.h -> StRoot/St_geant_Maker/AgStarReader.h

StRoot/StarGenerator/FILT/StarFilterMaker.cxx does not depend on anything in StRoot/StarGenerator/BASE/AgStarReader.h

plexoos · 2021-08-14T14:05:08Z

cons seems to work fine when I compile on rcf.

That is exactly the problem with the environment on rcf. You move/rename a file in your local directory but cons can still pick up the old header files from the central location pointed by $STAR

plexoos · 2021-08-16T03:50:31Z

StRoot/St_geant_Maker/St_geant_Maker.cxx

+    if ( stack != 0 )       AgStarReader::Instance().SetStack( (StarParticleStack*)stack->GetObject() );
+    if ( data  != 0 )       AgStarReader::Instance().SetParticleData( (StarParticleData*)data->GetObject() );


Per STAR coding guidelines C-style type casting should be avoided.

This is one of those cases where it is hard to avoid. If I use static / dynamic casts, then the dynamic linker complains that StarParticleStack / Data are not completely defined when I load the St_geant_Maker library. Same result if I push the cast down into the AgStarReader class. The crime against good coding style is not the use of the c-style cast, but rather the use of the singleton pattern for AgStarReader. Would prefer to keep the c-style cast here, rather than re-engineer the fortran interface.

Are you trying to avoid #include "StarGenerator/UTIL/StarParticleData.h" and #include "StarGenerator/BASE/StarParticleStack.h" from StRoot/St_geant_Maker/St_geant_Maker.cxx?

Yes. Also trying to avoid any run time dependence between the two libraries, so that
St_geant_Maker can be loaded without StarGenerator, and StarGenerator can be loaded w/out
loading St_geant_Maker.

(Also, not sure if replying in email gets logged as part of the discussion on github? If so, this may be redundant.)

StRoot/St_geant_Maker/St_geant_Maker.cxx

StRoot/StarGenerator/UTIL/StarParticleData.cxx

plexoos · 2021-08-16T04:01:23Z

StRoot/StarGenerator/macros/stargentest.py

+import datetime, os
+import ROOT
+import subprocess, shlex
+import sys


datetime, ROOT, and sys are not used

Removed... will push shortly.

StRoot/St_geant_Maker/St_geant_Maker.cxx

For clarity Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>

klendathu2k · 2021-08-16T22:11:54Z

Yes. Also trying to avoid any run time dependence between the two libraries, so that St_geant_Maker can be loaded without StarGenerator, and StarGenerator can be loaded w/out loading St_geant_Maker.

…

On 2021-08-16 16:29, Dmitri Smirnov wrote: @plexoos commented on this pull request. ------------------------- In StRoot/St_geant_Maker/St_geant_Maker.cxx [1]: > + if ( stack != 0 ) AgStarReader::Instance().SetStack( (StarParticleStack*)stack->GetObject() ); + if ( data != 0 ) AgStarReader::Instance().SetParticleData( (StarParticleData*)data->GetObject() ); Are you trying to avoid #include "StarGenerator/UTIL/StarParticleData.h" and #include "StarGenerator/BASE/StarParticleStack.h" from StRoot/St_geant_Maker/St_geant_Maker.cxx? -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4]. Links: ------ [1] #103 (comment) [2] https://github.com/notifications/unsubscribe-auth/ANL4LVES6PMW2PT2FFWMEWLT5FYMJANCNFSM5CEJDTWQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!XLCtpy9gZ0ca7HZlivJP7MRXJOrMg19dcTwCbf-TzjDCM8_c_groPphI8z7B7J0$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email__;!!P4SdNyxKAPE!XLCtpy9gZ0ca7HZlivJP7MRXJOrMg19dcTwCbf-TzjDCM8_c_groPphIH6_hHHk$

plexoos · 2021-08-17T00:36:20Z

Jason, Can you illustrate with a macro what you are trying to achieve. It is not very clear, how you expect to avoid a run time dependence if one library makes use of the other one. As far as I know, a dynamic loader does not complain about unresolved symbols but if you try to call something that depends on the second library the process will definitely crash.

klendathu2k · 2021-08-17T01:26:35Z

Any (most) of the macros under StRoot/StarGenerator/macros will demonstrate... e.g.

$ ln -s StRoot/StarGenerator/macros/starsim.pythia6.C starsim.C
$ root4star -q -b starsim.C
...
As implemented, code runs. To add a static or dynamic cast requires that I include the StarParticleStack and Data headers. Including those... I get this when I run starsim.C...

dlopen error: /gpfs/mnt/gpfs01/star/simu/simu/jwebb/STAR/.sl73_gcc485/lib/libSt_geant_Maker.so: undefined symbol: _ZTI16StarParticleData
Load Error: Failed to load Dynamic link library /gpfs/mnt/gpfs01/star/simu/simu/jwebb/STAR/.sl73_gcc485/lib/libSt_geant_Maker.so

$ c++filt _ZTI16StarParticleData
typeinfo for StarParticleData

plexoos · 2021-08-17T21:50:44Z

As implemented, code runs.

I could not reproduce this. When run from the current main your test goes through and produces an fzd file but from G4star it seg faults.

klendathu2k · 2021-08-17T23:16:27Z

I get a segmentation fault if I compile star-sw without compiling star-mcgen. Otherwise it worked (to be confirmed again...)

…cular dependency, meaning that it will not be simple to remove the starsim dependency in StarGenerator. We have two (or more) choices here. Either create an interface class and a separate library for the concrete starsim reader, or provide a set of dummy symbols in geant4 maker in order to satisfy the generator dependencies. Final choice will be defered, but I am leaning towards a set of dummy symbols in geant4mk.

klendathu2k · 2021-08-18T14:36:04Z

Decided on a different path. Instead of removing the dependence on starsim functions from StarGenerator (agsvert, agskine, etc...), the plan will be to satisfy these with dummy symbols inserted into the geant4 maker. Alternatively I could create an interface base class, and a plugin library to support starsim (but this seems like overkill).

klendathu2k · 2021-08-18T19:46:49Z

StRoot/StBFChain/BigFullChain.h

@@ -1363,6 +1363,15 @@ Bfc_st BFC[] = { // standard chains
  {"gstarLib","","",""                                                 ,"","gstar","Load gstar lib",kFALSE},
  {"flux"        ,"","","simu"                                           ,"","flux","Load flux lib",kFALSE},
  {"------------","-----------","-----------","------------------------------------------","","","",kFALSE},
+  {"Generators  ","-----------","-----------","------------------------------------------","","","",kFALSE},


DId not intend to commit this, but it is a harmless addition to our list of chain options (and will be utilized as we merge in the G4star codes).

If I understand correctly, if you update the branch from which you are making the PR not to have this line, then it will drop off the list of changed files. Then you can re-apply this change later when it is needed.

Can you clarify if it is only the single "Generators" line that isn't needed for this commit, or all the BigFullChain.h line changes in this PR that aren't really needed for this PR? git makes my approval mandatory for the whole shebang of this PR because I'm a code owner for a small part of it :-(

klendathu2k · 2021-08-18T21:26:50Z

Yes, that's my understanding as well. This would otherwise be in my next pull request, so not worth the effort to retract.

…

On 2021-08-18 17:18, Gene Van Buren wrote: @genevb commented on this pull request. ------------------------- In StRoot/StBFChain/BigFullChain.h [1]: > @@ -1363,6 +1363,15 @@ Bfc_st BFC[] = { // standard chains {"gstarLib","","","" ,"","gstar","Load gstar lib",kFALSE}, {"flux" ,"","","simu" ,"","flux","Load flux lib",kFALSE}, {"------------","-----------","-----------","------------------------------------------","","","",kFALSE}, + {"Generators ","-----------","-----------","------------------------------------------","","","",kFALSE}, If I understand correctly, if you update the branch from which you are making the PR not to have this line, then it will drop off the list of changed files. Then you can re-apply this change later when it is needed. -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4]. Links: ------ [1] #103 (comment) [2] https://github.com/notifications/unsubscribe-auth/ANL4LVFT7FFVHXSEDY4WMFTT5QPTHANCNFSM5CEJDTWQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!U2j1b4N6coeaMKudKwpWU9MNrggjtnAZqCBMRGP-7deYgbgQ9xia87Mk7eve5Mo$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email__;!!P4SdNyxKAPE!U2j1b4N6coeaMKudKwpWU9MNrggjtnAZqCBMRGP-7deYgbgQ9xia87MkaYeDij4$

klendathu2k · 2021-08-18T21:50:17Z

Hi Gene, It's the group of ~5 event generator declarations which follows (as well as that "Generators" header). Jason

…

On 2021-08-18 17:34, Gene Van Buren wrote: @genevb commented on this pull request. ------------------------- In StRoot/StBFChain/BigFullChain.h [1]: > @@ -1363,6 +1363,15 @@ Bfc_st BFC[] = { // standard chains {"gstarLib","","","" ,"","gstar","Load gstar lib",kFALSE}, {"flux" ,"","","simu" ,"","flux","Load flux lib",kFALSE}, {"------------","-----------","-----------","------------------------------------------","","","",kFALSE}, + {"Generators ","-----------","-----------","------------------------------------------","","","",kFALSE}, Can you clarify if it is only the single "Generators" line that isn't needed for this commit, or all the BigFullChain.h line changes in this PR that aren't really needed for this PR? git makes my approval mandatory for the whole shebang of this PR because I'm a code owner for a small part of it :-( -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4]. Links: ------ [1] #103 (comment) [2] https://github.com/notifications/unsubscribe-auth/ANL4LVEEYMW2MRKIOKNQZMTT5QRNNANCNFSM5CEJDTWQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!QuRI6NMej0dQEM_ZKJ4Y1zaIjY0DrpIazuYKRRxq-URkLvFp0iLIzpEWMQ4Ik1Y$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email__;!!P4SdNyxKAPE!QuRI6NMej0dQEM_ZKJ4Y1zaIjY0DrpIazuYKRRxq-URkLvFp0iLIzpEWG0Q-54g$

klendathu2k · 2021-08-23T17:32:55Z

Just pinging this. Are there any other recommendations to push these changes?

genevb · 2021-08-23T17:54:17Z

Just pinging this. Are there any other recommendations to push these changes?

As long as your other pull request (for which these chain options matter) and this one point to each other, I'm not too particular.

genevb

No opposition.

StRoot/StarGenerator/BASE/AgStarReader.h

plexoos · 2021-08-23T19:13:43Z

The description states that "This removes from StarGenerator the dependence on code in the starsim library." I don't see how the submitted changes do anything to achieve this. The original intention to move the code that depends on St_geant_Maker from StarGenerator/ looked more like the move in the right direction but it was reverted.

Please update the description and title to reflect what is really being changed.

plexoos · 2021-08-23T22:22:25Z

Thanks for updating the description Jason.

It will be difficult to eliminate the dependence on the agseventread_ (&etc...) methods in StarGenerator.

This one is still confusing because ageventread is defined in StarGenerator... I am probably missing something but where is ageventread called from?

klendathu2k · 2021-08-24T00:09:07Z

This is deep in the dark recesses of starsim... https://github.com/star-bnl/star-sw/pull/103/files#diff-60be39524263b2d623a8cb4328ae1ce1e44cbf95a429a2bfad492e80127ea853 The block staring around line 113. We ask comis for the address of the 'AgUSRead' subroutine. If it doesn't exist, we ask it for the address of the 'AgUSEvent' subroutine. If an address was found, we call the subroutine at that point (line 117).

…

On 2021-08-23 18:22, Dmitri Smirnov wrote: Thanks for updating the description Jason. > It will be difficult to eliminate the dependence on the > agseventread_ (&etc...) methods in StarGenerator. This one is still confusing because ageventread is defined in StarGenerator... I am probably missing something but where is ageventread called from? -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4]. Links: ------ [1] #103 (comment) [2] https://github.com/notifications/unsubscribe-auth/ANL4LVDVXKG6BQCMYD3OUALT6LC2ZANCNFSM5CEJDTWQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!QO32SI-N6f48rLhs8alaw5VHG1f9OMUFRN1hKoUn-FVkS3Sis4_zQZl3R7msDe8$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email__;!!P4SdNyxKAPE!QO32SI-N6f48rLhs8alaw5VHG1f9OMUFRN1hKoUn-FVkS3Sis4_zQZl3VvaQzCc$

plexoos · 2021-08-24T04:48:47Z

Without tests it is very hard to judge whether these changes will break anything or not...
For example, I can't tell if it is safe to remove the definition of agusread https://github.com/star-bnl/star-sw/pull/103/files#diff-dcecc0e93bbe5fcf050ef4b946135b831eec3686ed0b1606a8355c62fca004e0L30 when there seems to be another one defined in https://github.com/star-bnl/star-sw/blob/main/pams/sim/gstar/gstar_input.g#L84

Anyway, if you are confident about these changes we can merge... or maybe it can wait until introduction of geant4star when these preparatory changes will be actually used.

klendathu2k · 2021-08-24T14:22:41Z

Yes I am confident that the code functions and is correct. agusread --> agusevent is simply a renaming of a subroutine, plus a branch under a switch statement to enable the different path. The code runs. stargentest.py confirms that events are produced.

plexoos

The author claims that all changes are necessary for Geant4 integration and have been tested

asps/Simulation/starsim/agzio/agzkine.age

klendathu2k and others added 20 commits June 11, 2021 15:23

Update StPicoDstMaker.cxx

ba187a4

Update StRoot/StPicoDstMaker/StPicoDstMaker.cxx

a397637

Point is well taken. Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>

Merge branch 'star-bnl:main' into main

c3ccb18

Merge branch 'star-bnl:main' into main

e161802

Merge branch 'star-bnl:main' into main

20c3175

Merge branch 'star-bnl:main' into main

a1d3f34

Merge branch 'star-bnl:main' into main

1cbe68f

virtual dtor

81ad64b

Publish the stack for external maker consumption

b6e1d0b

Remove unecessary includes

aea89d0

Make event generator framework independent of the agstar reader.

75f710f

Give particle stack a more specific default name

6fa2da2

Name the primary maker's stack something different than default name

7dcc2f4

Remove unnecessary include from header file

c7c2ef5

Need the declaration

0030d8a

Remove duplicates...

6d883f6

Hide the static instance better

7b173b1

Hide the static instance of StarParticleData to avoid library depende…

201ce4d

…ncies

Minimal test of the 'big three' event generators (pythia6,pythia8,hij…

21837b4

…ing).

klendathu2k requested review from nigmatkulov, perevbnlgov and plexoos as code owners August 13, 2021 21:05

plexoos added 2 commits August 13, 2021 20:53

Prefix included header with path to its new location

b39edda

Should resolve the compiler error due to the moved file: StRoot/StarGenerator/BASE/AgStarReader.h -> StRoot/St_geant_Maker/AgStarReader.h

Remove unnecessary include

6c79bc4

StRoot/StarGenerator/FILT/StarFilterMaker.cxx does not depend on anything in StRoot/StarGenerator/BASE/AgStarReader.h

plexoos added 2 commits August 14, 2021 16:52

Correct syntax for function call operator overload

80c4b80

Fix misstyped int type

05b6c7e

plexoos reviewed Aug 16, 2021

View reviewed changes

Update StRoot/St_geant_Maker/St_geant_Maker.cxx

dca9284

For clarity Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>

klendathu2k added 2 commits August 18, 2021 09:42

Stack may be null if Clear is called before Init

f7ea1d8

klendathu2k added 2 commits August 18, 2021 12:24

Add chain option to load event generators / framework

c82b354

Neglected to include ... the include

1ac9c5e

klendathu2k requested a review from genevb as a code owner August 18, 2021 18:00

klendathu2k commented Aug 18, 2021

View reviewed changes

genevb reviewed Aug 23, 2021

View reviewed changes

veprbl reviewed Aug 23, 2021

View reviewed changes

StRoot/StarGenerator/BASE/AgStarReader.h Outdated Show resolved Hide resolved

Remove unecessary setter to a singleton data object

ed6c7cb

plexoos approved these changes Aug 24, 2021

View reviewed changes

veprbl reviewed Aug 25, 2021

View reviewed changes

asps/Simulation/starsim/agzio/agzkine.age Show resolved Hide resolved

veprbl approved these changes Aug 25, 2021

View reviewed changes

plexoos changed the title ~~G4star~~ Update StarGenerator and starsim codes in preparation for geant4star integration Aug 25, 2021

[skip ci] Fix white space

27fc5db

plexoos merged commit 63d4d8f into star-bnl:main Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update StarGenerator and starsim codes in preparation for geant4star integration #103

Update StarGenerator and starsim codes in preparation for geant4star integration #103

klendathu2k commented Aug 13, 2021 •

edited

plexoos commented Aug 14, 2021

plexoos Aug 16, 2021

klendathu2k Aug 16, 2021

plexoos Aug 16, 2021

klendathu2k Aug 17, 2021

plexoos Aug 16, 2021

klendathu2k Aug 16, 2021

klendathu2k commented Aug 16, 2021 via email

plexoos commented Aug 17, 2021

klendathu2k commented Aug 17, 2021

plexoos commented Aug 17, 2021

klendathu2k commented Aug 17, 2021

klendathu2k commented Aug 18, 2021

klendathu2k Aug 18, 2021

genevb Aug 18, 2021

genevb Aug 18, 2021

klendathu2k commented Aug 18, 2021 via email

klendathu2k commented Aug 18, 2021 via email

klendathu2k commented Aug 23, 2021

genevb commented Aug 23, 2021

genevb left a comment

plexoos commented Aug 23, 2021

plexoos commented Aug 23, 2021

klendathu2k commented Aug 24, 2021 via email

plexoos commented Aug 24, 2021

klendathu2k commented Aug 24, 2021

plexoos left a comment

		if ( stack != 0 ) AgStarReader::Instance().SetStack( (StarParticleStack*)stack->GetObject() );
		if ( data != 0 ) AgStarReader::Instance().SetParticleData( (StarParticleData*)data->GetObject() );

Update StarGenerator and starsim codes in preparation for geant4star integration #103

Update StarGenerator and starsim codes in preparation for geant4star integration #103

Conversation

klendathu2k commented Aug 13, 2021 • edited

plexoos commented Aug 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klendathu2k commented Aug 16, 2021 via email

plexoos commented Aug 17, 2021

klendathu2k commented Aug 17, 2021

plexoos commented Aug 17, 2021

klendathu2k commented Aug 17, 2021

klendathu2k commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klendathu2k commented Aug 18, 2021 via email

klendathu2k commented Aug 18, 2021 via email

klendathu2k commented Aug 23, 2021

genevb commented Aug 23, 2021

genevb left a comment

Choose a reason for hiding this comment

plexoos commented Aug 23, 2021

plexoos commented Aug 23, 2021

klendathu2k commented Aug 24, 2021 via email

plexoos commented Aug 24, 2021

klendathu2k commented Aug 24, 2021

plexoos left a comment

Choose a reason for hiding this comment

klendathu2k commented Aug 13, 2021 •

edited