Fix weighting bugs and trampoline pure virtual crash#142
Open
austinschneider wants to merge 5 commits into
Open
Fix weighting bugs and trampoline pure virtual crash#142austinschneider wants to merge 5 commits into
austinschneider wants to merge 5 commits into
Conversation
Port fix from upstream/sdm (dd0ad4a): compute available targets using the intersection list (consistent with how Injector does it), and guard against divide-by-zero when total_prob is zero.
Port naming convention from upstream/sdm (49d253b): - TotalDecayWidth(InteractionRecord) now means per-final-state width (was TotalDecayWidthForFinalState) - TotalDecayWidthAllFinalStates(InteractionRecord) is the sum over all channels (was TotalDecayWidth) - Same rename for TotalDecayLength/TotalDecayLengthForFinalState - TotalDecayWidth(ParticleType) is unchanged This matches the CrossSection naming convention where the method taking an InteractionRecord returns the value for the specific final state in the records signature.
…m C++ The pyDecay/pyCrossSection trampoline self member was never initialized during normal construction. When the Python object was garbage-collected while C++ held a shared_ptr, pybind11::get_override could no longer find the Python override, causing the pure virtual fallthrough. Fix with two layers of defense: 1. SELF_OVERRIDE macros now lazily initialize self via get_object_handle on first call. Once set, the mutable self member prevents the Python object from being collected (prevents future GC). 2. InteractionCollection pybinding constructors now use keep_alive to prevent GC of passed Decay/CrossSection Python objects for the lifetime of the collection. Together these eliminate the need for the DarkNewsTables Holder pattern and the manual reference-keeping in Weighter.py (though those remain as additional safety).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates decay-weighting and decay API semantics to align with cross-section conventions, and hardens the pybind11 trampoline/bindings to prevent “pure virtual call” crashes when Python subclasses are garbage-collected while still referenced from C++.
Changes:
- Fix weighting target selection to use intersection-based
GetAvailableTargets(...)and add atotal_prob == 0guard to avoid divide-by-zero inWeightingUtils. - Rename/realign Decay width/length APIs so
InteractionRecordoverloads represent per-final-state quantities and introduce*AllFinalStatesfor totals; update call sites, tests, and Python bindings accordingly. - Improve Python subclass lifetime handling via lazy trampoline
selfinitialization andpybind11::keep_aliveonInteractionCollectionconstructors.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/python/test_hnl_decays.py | Updates test to use renamed Decay API (TotalDecayWidth(record) for per-final-state). |
| resources/processes/DarkNewsTables/DarkNewsDecay.py | Renames DarkNews decay width methods to match new Decay naming (needs follow-up fix per comment). |
| projects/utilities/public/SIREN/utilities/Pybind11Trampoline.h | Adds lazy self handle initialization and new override macros (incl. reference-return variants). |
| projects/interactions/public/SIREN/interactions/pyDecay.h | Updates trampoline interface to new Decay width/length method names. |
| projects/interactions/public/SIREN/interactions/pyDarkNewsDecay.h | Updates DarkNews trampoline interface to new Decay width naming. |
| projects/interactions/public/SIREN/interactions/InteractionCollection.h | Renames collection decay total helpers to *AllFinalStates. |
| projects/interactions/public/SIREN/interactions/HNLDipoleDecay.h | Renames/realigns decay width method overrides with new convention. |
| projects/interactions/public/SIREN/interactions/HNLDecay.h | Renames/realigns decay width method overrides with new convention. |
| projects/interactions/public/SIREN/interactions/ElectroweakDecay.h | Renames/realigns decay width method overrides with new convention. |
| projects/interactions/public/SIREN/interactions/Decay.h | Updates Decay abstract interface: adds TotalDecayWidthAllFinalStates(record) and TotalDecayLengthAllFinalStates(record). |
| projects/interactions/public/SIREN/interactions/DarkNewsDecay.h | Updates DarkNewsDecay override signatures to the new Decay interface. |
| projects/interactions/private/test/HNLDecay_TEST.cxx | Updates C++ unit tests to use TotalDecayWidth(record) for per-final-state widths. |
| projects/interactions/private/pyDecay.cxx | Updates trampoline implementations to match renamed methods and use updated override macros. |
| projects/interactions/private/pyDarkNewsDecay.cxx | Updates DarkNews trampoline implementations to match renamed methods and updated override macros. |
| projects/interactions/private/pybindings/InteractionCollection.h | Adds keep_alive to constructors and renames exposed decay totals to *AllFinalStates. |
| projects/interactions/private/pybindings/HNLDipoleDecay.h | Updates Python bindings to expose TotalDecayWidthAllFinalStates and per-final-state TotalDecayWidth(record). |
| projects/interactions/private/pybindings/HNLDecay.h | Updates Python bindings to expose TotalDecayWidthAllFinalStates and per-final-state TotalDecayWidth(record). |
| projects/interactions/private/pybindings/ElectroweakDecay.h | Updates Python bindings to expose TotalDecayWidthAllFinalStates and per-final-state TotalDecayWidth(record). |
| projects/interactions/private/pybindings/Decay.h | Expands Decay Python API surface and binds the new width/length methods. |
| projects/interactions/private/pybindings/DarkNewsDecay.h | Updates DarkNewsDecay Python bindings to match the new *AllFinalStates API. |
| projects/interactions/private/InteractionCollection.cxx | Implements renamed decay total helpers and delegates to dec->TotalDecayWidthAllFinalStates(...). |
| projects/interactions/private/HNLDipoleDecay.cxx | Implements renamed width functions and updates internal callers to per-final-state TotalDecayWidth(record). |
| projects/interactions/private/HNLDecay.cxx | Implements renamed width functions and updates internal callers to per-final-state TotalDecayWidth(record). |
| projects/interactions/private/ElectroweakDecay.cxx | Implements renamed width functions and updates internal callers to per-final-state TotalDecayWidth(record). |
| projects/interactions/private/Decay.cxx | Adds TotalDecayLengthAllFinalStates implementation and realigns length computations with renamed widths. |
| projects/interactions/private/DarkNewsDecay.cxx | Adds TotalDecayWidthAllFinalStates(record) and updates FinalStateProbability to use per-final-state TotalDecayWidth(record). |
| projects/injection/public/SIREN/injection/Weighter.tcc | Updates to use InteractionCollection::TotalDecayLengthAllFinalStates. |
| projects/injection/private/WeightingUtils.cxx | Uses intersection-based available targets, switches to per-final-state decay length, and guards divide-by-zero. |
| projects/injection/private/Injector.cxx | Switches to per-final-state TotalDecayLength(record) when building decay probabilities. |
| projects/distributions/private/secondary/vertex/SecondaryPhysicalVertexDistribution.cxx | Uses TotalDecayLengthAllFinalStates when computing total interaction/decay competition. |
| projects/distributions/private/secondary/vertex/SecondaryBoundedVertexDistribution.cxx | Uses TotalDecayLengthAllFinalStates when computing total interaction/decay competition. |
| projects/distributions/private/primary/vertex/RangePositionDistribution.cxx | Uses TotalDecayLengthAllFinalStates in vertex sampling/probability. |
| projects/distributions/private/primary/vertex/PrimaryPhysicalVertexDistribution.cxx | Uses TotalDecayLengthAllFinalStates in vertex sampling/probability. |
| projects/distributions/private/primary/vertex/PrimaryBoundedVertexDistribution.cxx | Uses TotalDecayLengthAllFinalStates in vertex sampling/probability. |
| projects/distributions/private/primary/vertex/PointSourcePositionDistribution.cxx | Uses TotalDecayLengthAllFinalStates in vertex sampling/probability. |
| projects/distributions/private/primary/vertex/FixedTargetPositionDistribution.cxx | Uses TotalDecayLengthAllFinalStates in vertex sampling/probability. |
| projects/distributions/private/primary/vertex/ColumnDepthPositionDistribution.cxx | Uses TotalDecayLengthAllFinalStates in vertex sampling/probability. |
Comments suppressed due to low confidence (1)
resources/processes/DarkNewsTables/DarkNewsDecay.py:335
DarkNewsDecayin C++ calls the overloadedTotalDecayWidth(...)with bothParticleType(total width) andInteractionRecord(per-final-state). This Python override currently assumesrecordis always anInteractionRecordand will fail when invoked with aParticleType(e.g., viaDecay::TotalDecayWidth(ParticleType)), breaking calls likeTotalDecayWidthAllFinalStatesfallback paths. Update this method to dispatch on the argument type (handleParticleTypevsInteractionRecord), and consider makingTotalDecayWidthAllFinalStatestake only anInteractionRecordand delegate to theParticleTypecase, reusing the cachedself.total_widthrather than recomputing integrals.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The pyDecay trampoline called SELF_OVERRIDE_PURE with GetPossibleSignaturesFromParents (plural, the CrossSection method name) instead of GetPossibleSignaturesFromParent (singular, the Decay method name). This caused Python Decay subclasses to crash with 'pure virtual function called' when the C++ Injector tried to enumerate decay signatures.
…onTree GenerateEvent returns InteractionTree by value, but the pybind binding used the default copy policy. InteractionTree contains shared_ptr members that make the implicit copy constructor incompatible with pybind11 copy semantics. Changed to return_value_policy::move.
Comment on lines
300
to
333
| @@ -328,7 +328,7 @@ def TotalDecayWidth(self, arg1): | |||
| self.total_width = self.dec_case.total_width() | |||
| return self.total_width | |||
|
|
|||
| def TotalDecayWidthForFinalState(self, record): | |||
| def TotalDecayWidth(self, record): | |||
| sig = self.GetPossibleSignatures()[0] | |||
| if ( | |||
Comment on lines
+40
to
+41
| std::set<siren::dataclasses::ParticleType> available_targets_list = detector_model->GetAvailableTargets(intersections, DetectorPosition(record.interaction_vertex)); | ||
| std::set<siren::dataclasses::ParticleType> available_targets(available_targets_list.begin(), available_targets_list.end()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GetAvailableTargets(consistent with Injector) and guard against divide-by-zero whentotal_prob == 0TotalDecayWidth(InteractionRecord)toTotalDecayWidthAllFinalStatesandTotalDecayWidthForFinalStatetoTotalDecayWidth, matching the CrossSection convention where the InteractionRecord overload returns the per-final-state valueselfreference and addingkeep_aliveto InteractionCollection constructorsPorts/adapts fixes from sdm commits dd0ad4a, 49d253b, and introduces a proper fix for the trampoline lifetime issue that previously required the
DarkNewsTables.Holderworkaround.Test plan