Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow users of ddsim to set e.g. stepping action #1218

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Jan 25, 2024

For some studies we use DDG4 stepping (etc) action plugins in ddsim runs. For example, we are using this for fluence calculations by keeping track of where steps are activated.

This now allows, e.g.

ddsim --compactFile $DD4hepINSTALL/DDDetectors/compact/SiD.xml -G -N 10 --action.event "Geant4TestEventAction" --action.step "Geant4TestStepAction" --action.stack "Geant4TestStackAction" --printLevel VERBOSE

and the output indicates that the actions are called a suitably large number of times.

TODO:

  • figure out how to make repeated actions be additive instead of overwriting.
  • is there a benefit to put the JSON parsing into ConfigHelpers and make available for use elsewhere (filters are now not able to be specified on CLI)?
  • unit tests, in examples/CLICSiD/scripts/CLICSiDAClick.C which is only place that uses Geant4TestEventAction, but doesn't test ddsim calling interface?

BEGINRELEASENOTES

  • add command line options to pass stepping (etc) action plugins to ddsim

ENDRELEASENOTES

Copy link

github-actions bot commented Jan 25, 2024

Test Results

    6 files      6 suites   2h 18m 26s ⏱️
  360 tests   360 ✅ 0 💤 0 ❌
1 064 runs  1 064 ✅ 0 💤 0 ❌

Results for commit 292bcea.

♻️ This comment has been updated with latest results.

@wdconinc wdconinc marked this pull request as ready for review January 26, 2024 00:56
@andresailer
Copy link
Member

Thanks for this!

Is it likely or possible that one would like to configure properties for these actions?

@wdconinc
Copy link
Contributor Author

It probably should at least be possible, but because the plugins can be anything I don't know what the best way to pass these options would be. Is this best done with a dict?

@andresailer
Copy link
Member

For configuration something similar is done with the filters, if I remember correctly. Either just a name, like now, or a tuple of name and dict with properties.

@wdconinc
Copy link
Contributor Author

I'll implement this with parameters, otherwise it's pretty limited in its usefulness I'm realizing.

@wdconinc
Copy link
Contributor Author

wdconinc commented Feb 4, 2024

I added the ability to specify parameters, and modified Geant4TestActions's Geant4TestBase to print out its properties on destructions to be able to test this. Now we can do:

ddsim --compactFile $DD4hepINSTALL/DDDetectors/compact/SiD.xml -G -N 10 --action.event '{ "name": "Geant4TestEventAction/EventAction0", "parameter" : {"Property_int": 10} }' --action.stack '{ "name": "Geant4TestStackAction/StackAction1", "parameter" : {"Property_int": 11} }' --printLevel VERBOSE

and it will print

...
EventAction0     VERB  Geant4Action: Deleting object EventAction0 of type dd4hep::sim::Test::Geant4TestEventAction Pointer:0x561856e3d560
Geant4TestEventAction VERB  properties at destruction: 10, 0.000000, 
...
StackAction1     VERB  Geant4Action: Deleting object StackAction1 of type dd4hep::sim::Test::Geant4TestStackAction Pointer:0x561856ef63d0
Geant4TestStackAction VERB  properties at destruction: 11, 0.000000, 
...

What I haven't been able to make work yet is something with two event actions, i.e. like this:

ddsim --compactFile $DD4hepINSTALL/DDDetectors/compact/SiD.xml -G -N 10 --action.event '{ "name": "Geant4TestEventAction/EventAction0", "parameter" : {"Property_int": 10} }' --action.event '{ "name": "Geant4TestEventAction/EventAction1", "parameter" : {"Property_int": 11} }' --printLevel VERBOSE

This just activates the last action only. I'm probably overlooking something obvious, but I'll look again later. For now, adding multiple event actions through a list is also possible:

ddsim --compactFile $DD4hepINSTALL/DDDetectors/compact/SiD.xml -G -N 10 --action.event '[ { "name": "Geant4TestEventAction/EventAction0", "parameter" : {"Property_int": 10} }, { "name": "Geant4TestEventAction/EventAction1", "parameter" : {"Property_int": 11} } ]' --printLevel VERBOSE

which does give

...
EventAction0     VERB  Geant4Action: Deleting object EventAction0 of type dd4hep::sim::Test::Geant4TestEventAction Pointer:0x55838d449fc0
Geant4TestEventAction VERB  properties at destruction: 10, 0.000000, 
...
EventAction1     VERB  Geant4Action: Deleting object EventAction1 of type dd4hep::sim::Test::Geant4TestEventAction Pointer:0x55838d4bd150
Geant4TestEventAction VERB  properties at destruction: 11, 0.000000, 
...

@wdconinc wdconinc force-pushed the ddsim-set-run-event-track-step-stack-actions branch from 1c926ae to 83a89d3 Compare February 4, 2024 04:07
for parameter, value in action_dict['parameter'].items():
setattr(action, parameter, value)
kernel_Action().add(action)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may want to kernel.registerGlobalAction(action) here to allow access by name later on(?).

Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>
DDG4/python/DDSim/Helper/Action.py Show resolved Hide resolved
DDG4/python/DDSim/Helper/Action.py Outdated Show resolved Hide resolved
DDG4/python/DDSim/DD4hepSimulation.py Outdated Show resolved Hide resolved
DDG4/python/DDSim/Helper/Action.py Outdated Show resolved Hide resolved
DDG4/python/DDSim/Helper/Action.py Outdated Show resolved Hide resolved
wdconinc and others added 2 commits February 6, 2024 08:07
@wdconinc
Copy link
Contributor Author

Sorry for the slow progress on this (heavy teaching load this term).

This now passes the unit test, BUT

  • CTest is unable to check for multiple required regex patterns in the output: set_tests_properties(PASS_REGULAR_EXPRESSION ${list}) passes as soon as one match is found; since order is not guaranteed, I was going with a negative lookahead (i.e. (?=Deleting object RunAction1)(?=Deleting object EventAction1), but apparently CMake implements its own regex parser without support for negative lookahead. No idea how to implement a unit test that asserts multiple distinct lines should be present in the output... So, the unit test is just testing whether it runs, not that it does all that's expected based on the command line arguments.
  • Due to the above, the unit test doesn't catch the following:
    • an option specified in the steering file and NOT on the command line is duplicated (applies to RunAction1 in the unit test); it gets added first in the exec of the steering file, and then again by __parseAllHelper.
    • an option specified multiple times on the command line only gets added for the last instance (applies to StepActionCLI1, which isn't added while StepActionCLI3 is added).

@wdconinc
Copy link
Contributor Author

After c452e5a no more duplication of inputs, and no more missing arguments.

@MarkusFrankATcernch MarkusFrankATcernch merged commit ade128c into AIDASoft:master Feb 12, 2024
14 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.

3 participants