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

Fix a crash when an action has dictionaries and is not hashable #1293

Closed
wants to merge 2 commits into from

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Jul 11, 2024

Fixes #1292, introduced first in #1288. The issue happens when passing an action that has a dictionary, like in https://github.com/key4hep/k4geo/blob/2e1bba216879562382ea5599b738c14c0938b540/example/steeringFile.py#L22.

BEGINRELEASENOTES

  • Fix a crash when passing to ddsim an action has dictionaries and is not hashable.
  • Refactor the functions in DDG4.py to setup a calorimeter and a tracker detector into a single one since the logic is the same. In this new function, use a try-except and if the type is not hashable use the default value.

ENDRELEASENOTES

Refactor the functions to setup a calorimeter and a tracker detector into a
single one since the logic is the same. In this new function, use a
try-except and if the type is not hashable used the default value.
Copy link

github-actions bot commented Jul 11, 2024

Test Results

   12 files     12 suites   7h 11m 35s ⏱️
  360 tests   360 ✅ 0 💤 0 ❌
2 148 runs  2 148 ✅ 0 💤 0 ❌

Results for commit 86e370f.

♻️ This comment has been updated with latest results.

@andresailer
Copy link
Member

Can you add some mention of ddsim into the release notes part?

use a try-except and if the type is not hashable used the default value

I am not sure that should be done. Why is a non-hashable type given in the first place?
Probably, we shouldn't silently ignore this input, but either refuse to do something, or make it work as intended by the user.

@jmcarcell
Copy link
Member Author

jmcarcell commented Jul 19, 2024

So having another look at this, I updated the logic and the parameters are being set here: https://github.com/AIDASoft/DD4hep/blob/master/DDG4/python/DDG4.py#L664. I'm not sure how to test if a sensitive action is running though, the example in k4geo with the crash seems to create it but not actually run it.
In summary:

@MarkusFrankATcernch
Copy link
Contributor

Change in PR 1288 reverted. See PR 1301.

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.

k4geo CI failing, potentially caused by last DD4hep PR
3 participants