search

star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

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

Closed klendathu2k closed 2 years ago

klendathu2k commented 2 years ago

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

plexoos commented 2 years ago

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

klendathu2k commented 2 years ago

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] https://github.com/star-bnl/star-sw/pull/103#discussion_r689837978 [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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

klendathu2k commented 2 years ago

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 commented 2 years ago

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] https://github.com/star-bnl/star-sw/pull/103#discussion_r691611174 [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 commented 2 years ago

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] https://github.com/star-bnl/star-sw/pull/103#discussion_r691620088 [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 commented 2 years ago

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

genevb commented 2 years ago

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.

plexoos commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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] https://github.com/star-bnl/star-sw/pull/103#issuecomment-904174734 [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 commented 2 years ago

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 commented 2 years ago

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.