Repository navigation
madspace: bind LHEEvent.alpha_qcd to the right member - #87
Merged
Merged
Conversation
The pybind11 binding for LHEEvent exposed `alpha_qcd` as `&LHEEvent::process_id`, so reading `event.alpha_qcd` from Python returned the (int) process id, and assigning a float to it raised TypeError. Point it at `&LHEEvent::alpha_qcd`. Add two regression tests: one asserting every LHEEvent attribute reads back its own value through both the constructor and the setters, and one checking the AQCDUP column of the event header line written by LHEFileWriter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
I guess that this should be merge quickly. |
Contributor
|
looks good |
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.
The bug
madspace/src/python/madspace.cppboundLHEEvent.alpha_qcdto&LHEEvent::process_id:The correct member is
&LHEEvent::alpha_qcd(double, declared inmadspace/include/madspace/driver/lhe_output.hpp:61). Present since the line was first written (391f807c3, "started implementing LHE output") — never correct.Observed from Python before the fix:
The generated stub was self-contradictory too:
__init__(..., alpha_qcd: SupportsFloat)next toalpha_qcd -> int. It regenerates correctly now.Blast radius
Nothing consumed it.
alpha_qcdis only ever read/written through the C++ struct (event_generator.cpp,lhe_output.cpp,io.hpp), never through the Python attribute — no use inmadspace/tests, MadGraph7'smg7templates, MadSpin, or the acceptance tests. LHE files produced by the normalcombine_to_lhepath were and are correct, since that never touches the binding. The reachable hazard was the Python-facing path only: anLHEEventbuilt or edited from Python and handed toLHEFileWriter.The two members have different types (
doublevsint), so the mis-binding was partly self-announcing on the setter (TypeErroron any float) but silent on the getter — reads just returned the process id.Audit of the surrounding bindings
Checked all 167
def_readwrite/def_readonlybindings inmadspace.cppmechanically (Python name vs. bound member name, and bound class vs. enclosingpy::classh). This was the only mismatch. Also hand-checked thepy::initargument names and orders of the whole LHE block —LHEHeader,LHEProcess,LHEMeta,LHEParticle,LHEEvent,SubprocArgs— against their struct declarations; all correct.Tests
Two additions to
madspace/tests/test_lhe.py, both failing before the fix and passing after:test_event_attributes_are_bound_to_their_own_members— everyLHEEventattribute reads back its own value, via the constructor and via the setters (catches any future aliasing in that block, not just this field).test_event_header_line_written_to_lhe— sets the fields one by one and checks theNUP IDPRUP XWGTUP SCALUP AQEDUP AQCDUPheader line of the file written byLHEFileWriter.They need the built extension, which is exactly what CI already does:
.github/workflows/madspace.ymlrunspytest {project}/madspace/testsagainst the cibuildwheel-built wheel. No CI changes needed.Suite
pytest madspace/tests(local, macOS arm64, Python 3.14):The 4 failures are pre-existing and need
scipy(test_double_t.py);test_flow.py/test_mlp.pyneedtorch. Neither is installed locally, both are installed in CI.🤖 Generated with Claude Code