Repository navigation
madspace: spell the LHEParticle momentum kwargs like its attributes (px/py/pz) - #88
Merged
Merged
Conversation
LHEParticle's constructor took p_x/p_y/p_z while the attributes it sets are px/py/pz, so a caller wrote LHEParticle(p_x=...) and read particle.px. px/py/pz is the spelling used everywhere else: the C++ struct members (lhe_output.hpp), the npy field layout and accessors (io.hpp), the observable enum (obs_px) and every kernel. The three py::arg names were the only place in madspace spelled p_x -- they are the anomaly, not the attributes. This follows 20b692d, which renamed the public kwarg colour_order to color_order the same way: a straight rename, no alias, updating the one test that used it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@theoheimel |
Contributor
|
Always good to have consistent naming. Happy to merge this |
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.
LHEParticle's constructor tookp_x/p_y/p_zwhile the attributes it sets arepx/py/pz, so a caller wroteLHEParticle(p_x=...)and read backparticle.px. This renames the three kwargs to match.Which side was the anomaly
The attributes, it turns out, are the ones that were already right.
px/py/pzis what the rest of madspace uses:double px, py, pz, energy, mass;(madspace/include/madspace/driver/lhe_output.hpp:50){"px", ...},px()(madspace/include/madspace/driver/io.hpp:60,75,96)obs_pxchili.hpp,rambo.hpp,observables.hpp,kinematics.hpp)Across the whole repo,
p_xas a madspace identifier appeared only on those threepy::arglines and the single test caller. Checking all 451py::argsites inmadspace.cppagainst their classes'def_readwrite/def_readonlynames,LHEParticleis the only class with a kwarg/attribute spelling split —LHEEvent,LHEMeta,LHEProcessandSubprocArgsare all consistent.Is this breaking, and who does it affect
Yes,
p_x=stops working. In practice the blast radius is one line:LHEParticleis constructed from Python in exactly one place,madspace/tests/test_lhe.py:132, updated here. The mg7 production path (madevent.py,gridpack.py) goes throughcombine_to_lhe, which builds particles C++-side and never names these kwargs. MadSpin does not touch it. Same on all 60origin/*branches: every one hasp_x=on that one test line and nowhere else..pyidoes carryp_xin the public signature, so this is a real public API rather than something repo-internal. Anyone pinned tomadspace==0.1.3keeps the old wheel; the break lands whenever the next release is cut (the in-repo version is still0.1.3, so a bump is needed before publishing regardless).Why a rename rather than accepting both spellings
A second
py::initwith thepxnames does work under pybind's two-pass overload resolution, but it is not clean here: it publishes two near-identical 13-argument overloads inhelp(), in everyTypeErrormessage and in the generated.pyi, and a mixed call likeLHEParticle(px=1., p_y=2.)fails both overloads with a doubly-confusing error. That is a worse public API than either single spelling.The deciding factor is that the project has already made this exact call. 20b692d ("Update spelling of color and add test dependencies to toml") renamed the public pybind kwarg
colour_order→color_order— the same kind of pure spelling normalisation — as a straight rename with no alias and no deprecation, updating the one test that used it. madspace has no deprecation machinery anywhere. This follows that precedent, which is also why it targetsmainrather than waiting for a release or development branch.The checked-in
.pyiis generated at build time bygenerate_pyi.py, so it picks the new names up automatically; nothing to hand-edit.Test
test_particle_momentum_kwargs_match_attributesinmadspace/tests/test_lhe.pypins the constructor topx/py/pzand assertsp_x/p_y/p_zare rejected. Before the change it fails withTypeError: __init__(): incompatible constructor arguments(along with the 12 tests using the updatedbuild_eventhelper).pytest madspace/tests: 1475 passed, 2 skipped. That reconciles with the 1470-passmainbaseline as 1470 + 4 (thetest_double_t.pyscipy tests, which pass here because this env has scipy 1.18.1) + 1 (the new test).test_flow.py/test_mlp.pyremain uncollectable without torch.Relation to the other open madspace PRs
Touches the same file as #87 but not the same lines (#87 is
LHEEvent'salpha_qcdbinding at ~1622, this isLHEParticle's kwargs at ~1584), so the two merge independently in either order. Independent of #86, which targets the MadSpin perf branch.🤖 Generated with Claude Code