Patch broken DarkNews ModelContainer#107
Conversation
Squashed diff for paths: - python/SIREN_DarkNews.py Source commits on dev/HNL_DIS_clean that contributed: 521695f (Nicholas Kamp): fix issues with 2D tabular integral calculation, sampling errors with very short decay lengths, DarkNews errors, and kinematic erros in Dipole portal HNL DIS ac0ce24 (Nicholas Kamp): changes to facilitate hadronic and W/Z HNL decays f183191 (Nicholas Kamp): more complex logic for adding darknews interactions, fix some errors in SIREN_DarkNews
b946500 to
a1ecea2
Compare
0389ba5 to
de7e5b1
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Patches DarkNews decay serialization/integration handling to fix broken ModelContainer behavior in the stacked HNL DIS refactor.
Changes:
- Adds
decay_dictto stored decay state and serialization output. - Updates pickle deserialization to load both
decay_dictanddecay_integrator. - Adjusts decay-width computation and DarkNews sampling input shape.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if os.path.isfile(int_file): | ||
| with open(int_file, "rb") as ifile: | ||
| _, self.decay_integrator = pickle.load(ifile) | ||
| self.decay_dict, self.decay_integrator = pickle.load(ifile) |
There was a problem hiding this comment.
pickle.load(ifile) is now assumed to return exactly a 2-tuple (decay_dict, decay_integrator). If existing tables were written by older code that pickled (_, decay_integrator) (or only the integrator), this will raise a ValueError at runtime and break users with cached tables. Consider supporting both formats (e.g., branch on tuple length / type) and initialize self.decay_dict appropriately when loading legacy pickles.
| self.decay_dict, self.decay_integrator = pickle.load(ifile) | |
| decay_data = pickle.load(ifile) | |
| if isinstance(decay_data, tuple) and len(decay_data) == 2: | |
| self.decay_dict, self.decay_integrator = decay_data | |
| else: | |
| # Backward compatibility with legacy caches that stored | |
| # only the integrator object. | |
| self.decay_dict = None | |
| self.decay_integrator = decay_data |
| # serialization method | ||
| def get_representation(self): | ||
| return {"decay_integrator":self.decay_integrator, | ||
| "decay_dict":self.decay_dict, |
There was a problem hiding this comment.
Adding the new "decay_dict" field changes the serialized representation schema. If there is any corresponding deserialization path (e.g., from_representation / model container loader) that expects a fixed set of keys, it may break when reading older saved state (missing key) or when older code reads newer state (unexpected key). A tangible mitigation is to make (de)serialization tolerant: treat "decay_dict" as optional with a default, and/or include a simple schema/version tag in the representation.
| "decay_dict":self.decay_dict, |
| try: | ||
| os.makedirs(self.table_dir, exist_ok=False) | ||
| print("Directory '%s' created successfully" % self.table_dir) | ||
| #print("Directory '%s' created successfully" % self.table_dir) |
There was a problem hiding this comment.
Commenting out the print leaves dead code in a runtime branch. Prefer either removing the line entirely, or switching to a proper logger call (at an appropriate level) so behavior can be controlled without editing code.
| # Expand dims required to call DarkNews function on signle sample | ||
| four_momenta = get_decay_momenta_from_vegas_samples( | ||
| np.expand_dims(PS, 0), | ||
| np.expand_dims(PS, 0).T, |
There was a problem hiding this comment.
np.expand_dims(PS, 0).T changes the shape from (1, N) to (N, 1) (and does nothing for 1D inputs beyond swapping axes convention). If get_decay_momenta_from_vegas_samples expects the original orientation, this will silently change semantics and can produce incorrect momenta. A more robust approach is to reshape explicitly to the exact expected shape (e.g., (1, -1) or (-1, 1)) and optionally validate the resulting dimensions before calling DarkNews.
a1ecea2 to
27c1fa6
Compare
de7e5b1 to
7ad4c57
Compare
27c1fa6 to
6da3cb1
Compare
7ad4c57 to
7b61952
Compare
6da3cb1 to
39252ee
Compare
7b61952 to
d107b83
Compare
39252ee to
daab60e
Compare
d107b83 to
485176b
Compare
|
Closing to reopen from the commit author's account so they can be added as a reviewer. Branch unchanged; will be re-linked from the new PR shortly. |
Stack: PR 9 of 14
This PR is part of a 14-PR stack decomposing
dev/HNL_DISinto reviewable chunks.feat/detector-sector-by-namefix/darknews-modelcontainerMerge order:
feat/detector-sector-by-nameshould land before this PR.Squashed contents
Squashed diff for paths:
Source commits on dev/HNL_DIS_clean that contributed:
4745193 (Austin Schneider): Patch broken DarkNews ModelContainer
521695f (Nicholas Kamp): fix issues with 2D tabular integral calculation, sampling errors with very short decay lengths, DarkNews errors, and kinematic erros in Dipole portal HNL DIS
ac0ce24 (Nicholas Kamp): changes to facilitate hadronic and W/Z HNL decays
f183191 (Nicholas Kamp): more complex logic for adding darknews interactions, fix some errors in SIREN_DarkNews
Notes
dev/HNL_DIS_clean..fitsdata files have been removed from the branch and are packaged separately.