refactor: take the tree options as one group (F09: 45 → 40 parameters) - #258
Merged
Conversation
Third group of F09's interface half, and the first where the default-parity check actually caught something. TreeConfig.tree_type defaults to None; this constructor defaulted to "radix". A straight swap would have handed every caller who omits the group tree_type=None -- silent, and not something any test asserts directly. None in the config means "not overridden", so the default is resolved at the unpack, with the same or "radix" the facade already applies at its construction call. That line is the whole of the fix and it is commented as such. Three groups in, one mismatch, and it was on the third. That is the argument for running the check every time rather than after being surprised. The mapping is also the widest so far -- mode for tree_build_mode, leaf_target for target_leaf_particles -- so it is stated in full at the unpack and in the docstring, and the mapping was asserted complete with no TreeConfig field left unmapped. A second lesson, from a mistake I made here: converting dict(...) builders is only correct when the dict feeds an ENGINE construction. My sweep converted every dict(...) carrying a matching key, which also hit three feeding derive_split_build_default, resolve_dehnen_geometry and the FACADE (FastMultipoleMethod, which takes flat kwargs) -- 9 unit failures, all after the golden had already passed. Reverted those three; check the consumer, not just the keyword. The golden is unmoved and its data file unmodified. Its two tree matrix cases now build a TreeConfig; case names unchanged. 1521 passed, 119 skipped; characterization 34 passed; pydoclint 0; JACCPOT_RUNTIME_TYPECHECK=1 pytest tests/unit green at 1264 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Third group of F09's interface half: 45 → 40 parameters. Branched off main, not stacked.
The parity check finally caught something
TreeConfig.tree_typedefaults toNone; the constructor defaulted to"radix".A straight swap would have handed every caller who omits the group
tree_type=None— silent,and not something any test in the suite asserts directly. The golden wouldn't have caught it
either, since its tree cases pass an explicit mode.
Resolved at the unpack with the same
or "radix"the facade already applies at its ownconstruction call:
Three groups in, one mismatch, and it was on the third. That's the argument for running the
check every time rather than after being surprised by one.
This mapping is also the widest so far —
modefortree_build_mode,leaf_targetfortarget_leaf_particles— so it's written out at the unpack and in the docstring, and assertedcomplete with no
TreeConfigfield left unmapped.A mistake worth recording
Converting
dict(...)builders is only correct when the dict feeds an engine construction.My sweep converted every
dict(...)carrying a matching key, which also hit three feedingderive_split_build_default,resolve_dehnen_geometry, and the facade (FastMultipoleMethod,which takes flat kwargs) — 9 unit failures, all surfacing after the golden had already passed.
Reverted those three. The rule for the last group: check the consumer, not just the keyword.
That's now twice this refactor that the golden went green while call sites were broken. It
verifies resolved state; it says nothing about whether every caller still constructs.
Verification
Remaining
FarFieldConfig— 9 params across 39 sites, the last group and the largest. 63 → 40 so far;that one would take it to ~32.
🤖 Generated with Claude Code