fix: second wave of #1578 fixes — 43 items across processor, nanoevents, analysis/jetmet/lookup/lumi/ml/btag tools, CI, and dead code - #1597
Draft
NJManganelli wants to merge 66 commits into
Conversation
added 30 commits
July 7, 2026 11:06
…test-dask-client Without if: always() plus a result check, a skipped required job let the pass check go green; release also omitted test-dask-client from needs. Assisted-by: Claude Opus 4.8
treat_pt_as_raw was computed after ptRaw was inserted into the name map, so it was always False, and the massRaw fallback overwrote ptRaw instead of setting massRaw, crashing build() when raw mappings were omitted. Assisted-by: Claude Opus 4.8
Seeding indexed the first and last elements of the input, raising IndexError for length-zero arrays; fall back to a fixed seed since no random numbers are drawn anyway. Assisted-by: Claude Opus 4.8
Setting the 'corrected' parameter wrote into the parameter dict shared with the caller's jets layout; copy it before modifying. Assisted-by: Claude Opus 4.8
Assisted-by: Claude Opus 4.8
…works again numpy.subtract was never registered for candidate-derived classes, so subtracting any two NanoAOD candidates raised TypeError since v2024.8.0. Mirrors Candidate.add, differencing charge. Assisted-by: Claude Opus 4.8
…egates the time component pt/eta/phi/mass cannot represent t < 0, so scaling by a negative number now returns a cartesian LorentzVector matching LorentzVector.multiply; negative() delegates to multiply(-1). Assisted-by: Claude Opus 4.8
…minate The soft-terminate signal handler began with an unconditional `raise KeyboardInterrupt`, aborting the whole run and discarding completed results on the first C-c instead of cancelling remaining tasks and accumulating what finished. Assisted-by: Claude Opus 4.8
`concurrent_reads` was passed as the third positional arg of `_compression_wrapper`, which is `name`, so the option was silently ignored and accumulation always used the default of 2 reader threads. Bind it into `accumulate_result_files` via functools.partial. Assisted-by: Claude Opus 4.8
WeightStatistics.add() mutates in place and returns None, so __add__ and __iadd__ returned None. 'ws += other' rebound ws to None, poisoning any accumulation over processor outputs. Assisted-by: Claude Opus 4.8
The option-type check in __add_multivariation_delayed tested the dask array object itself instead of its awkward type, so it never fired and missing weights were left unfilled -- unlike every other add path (eager and the delayed single-variation path) which fill None with 1.0. Assisted-by: Claude Opus 4.8
…ariation names
partial_weight() validated modifiers with replace('Down','').replace('Up','')
against the weight-name set, which (a) rejected every add_multivariation()
modifier -- their base name is 'weight_modifier', not a stored weight key --
and (b) mangled any weight whose name contains 'Up'/'Down' mid-string.
Switch weight(), partial_weight() and variations to strip/replace only the
trailing Up/Down suffix, and validate multivariation modifiers by owning weight.
Assisted-by: Claude Opus 4.8
…ask copy require() was lru_cached and handed the same array to every caller, so an in-place op (e.g. mask &= other) corrupted the cached mask for later callers. Move the computation to a cached _require and return a copy from require. Assisted-by: Claude Opus 4.8
numpy.int was removed in numpy>=1.24, crashing convert_effective_area_file on .ea.txt files with 2+ binned variables. Assisted-by: Claude Opus 4.8
The documented .pileup.json path emitted a "json_lookup" type that was absent from evaluator.lookup_types (KeyError in make_evaluator), and the extractor overwrote the name-table index to 0, aliasing the pileup weight onto an unrelated entry. Restore the json_lookup class, register it, and drop the index override. Assisted-by: Claude Opus 4.8
AssociatedSV.jet indexed events with the Jet array instead of the collection name, crashing in every mode. AssociatedPFCand.jet's dask path called dask_array.events() instead of _events(). Both now match their sibling properties. Assisted-by: Claude Opus 4.8
…ed bytes Fixes bug 11 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
…root kwarg Fixes bug 21 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
Fixes bug 22 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
Fixes bug 25 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
Fixes bug 47 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
Fixes bug 56 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
Fixes bug 55 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
Fixes bug 24 from scikit-hep#1578. Assisted-by: Claude Opus 4.8
The _lookup_branch OneToManyRelations branch fetched the "OneToOneRelations" key from the datatype definition, so one-to-many members resolved to wrong types and their subcollections were skipped. Assisted-by: Claude Opus 4.8
The unknown-collection RecordArray path referenced a nonexistent self._datatype_mixins (drift from the edm4hep.py copy), raising AttributeError whenever a leftover record branch was processed. Assisted-by: Claude Opus 4.8
FCC.get_schema silently returned None for unrecognized versions; it now raises ValueError listing the valid versions. Assisted-by: Claude Opus 4.8
NanoEventsFactory.__getstate__/__setstate__ dropped _mode, so an unpickled factory raised AttributeError from events(). Assisted-by: Claude Opus 4.8
from_preloaded never passed buffer_cache to PreloadedSourceMapping, and PreloadedSourceMapping forwarded it into BaseSourceMapping's file_handle slot positionally, so the cache was silently dropped. Assisted-by: Claude Opus 4.8
get_index_ranges used awkward.sum(ranges) == 0 to detect empty ranges, which also fired when the only produced indices were zeros, replacing real data with a twice-nested empty array. Count elements instead. Assisted-by: Claude Opus 4.8
added 22 commits
July 8, 2026 15:04
Removes the plain nested_local2global (never dispatched: no form emits the "!nested_local2global" token; only nested_local2global_stack/_form are used) and the internally-broken begin_end_mapping_with_xyzrecord along with its exclusive helpers get_array_from_indices_xyzrecord_target(_kernel). The latter is never invoked because begin_end_mapping_with_xyzrecord_form emits the "!begin_end_mapping" token rather than "!begin_end_mapping_with_xyzrecord". The still-used _form variant is retained. Assisted-by: Claude Opus 4.8
Removes the unused _generate_slices helper and four unreachable NotImplementedError branches in NminusOne/Cutflow yieldhist methods: each sits inside an `if not ... and not do_categorical:` block, so `categorical is not None` (i.e. do_categorical) can never be true there. Assisted-by: Claude Opus 4.8
Removes dense_evaluated_lookup: no converter ever produces the "dense_evaluated_lookup" type string, so the evaluator registry entry was unreachable. Also removes the unused flatten_idxs helper in jme_standard_function.py (never called; also relied on the removed numpy.int). Assisted-by: Claude Opus 4.8
Removes the unused _hex and _ascii helpers, which had no references anywhere in the codebase. Assisted-by: Claude Opus 4.8
The edm4hep yaml asset was re-read from disk, deepcopied, and fully parsed on every file/schema build. Cache the (raw, parsed) dicts per version at module level via lru_cache; the schema only reads them, so a single parse is safely shared. Also reuse the all_collections set already computed in _create_mixin instead of recomputing it in _build_collections. Behavior is bit-identical (existing edm4hep + fcc suites pass). Adds a guard test asserting the shared cache is treated read-only. Assisted-by: Claude Opus 4.8
Building the dense correction lookup ran an O(cells x bins) pure-Python double loop per systematic (findbin + ndenumerate). Replace it with a vectorized numpy broadcast: match each grid cell against all bins at once, take the first match via argmax, and apply the abseta fallback the same way. Results are exactly equal (verified: 157 mapping arrays / 201,204 cells across 5 sample CSVs, bit-identical). Assisted-by: Claude Opus 4.8
# Conflicts: # tests/test_nanoevents_edm4hep.py
…sted_local2global, deprecate tailtimeout instead of removing GitHub code search shows external analyses pass tailtimeout to the executors and depend on dense_evaluated_lookup (user-maintained converters) and nested_local2global (custom schemas); internal-only reference proofs are insufficient for public API. tailtimeout stays as an ignored field that warns DeprecationWarning when set. Assisted-by: Claude Opus 4.8
Collaborator
Author
|
I guess we need an adversarial review, and we also want to wait for wave 1 fixes to be merged in first, yeah? |
Collaborator
|
Yeah it would be best to get everything in and then do the review. |
…ts by behavior Drop the issue-tracker references from the two vector tests introduced by this wave; state what each verifies about the current behavior. Assisted-by: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XeYa8sEdeLGa1VX2frvoNz
ikrommyd
marked this pull request as draft
July 17, 2026 23:11
Member
|
Converted this to draft for safety |
Member
|
I would love to split out the individual fixes into separate PRs or group fixes that make sense to be together at least. This currently has too many unrelated changes. |
Collaborator
Author
|
I created 3 variations of PRs, the omnibus had gotten the previous votes, but if you want to look at/argue for the reduced-group, see here: https://github.com/NJManganelli/coffea/tree/fix/1578-planB-cleanup I did not push the more factorized fixes, which was around a dozen total for the 3 dozen-ish individual issues big and small But first, as already agreed, we need to get the other fixes in. Please have a look at those. I did another pass the reduce the verbosity of Claude. |
NJManganelli
force-pushed
the
integration/1578-wave-fixes
branch
from
July 25, 2026 16:45
88da398 to
c7c8a2f
Compare
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
3 participants
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.
Part of #1578. This integrates 15 individual small-batch fixes
First, here's the enumerate copy of Henry's review, for easier checks.
raise KeyboardInterruptmade soft-terminate dead;concurrent_readsswallowed bynameparam_mode; preloadedbuffer_cachedropped; zero-valued index ranges →[[]]; codec corruption of non-contiguous arrays; needless numcodecs requirement; double decode per cache hitAssociatedSV.jet/AssociatedPFCand.jetcrashes; polar negative-scalar multiply now matches cartesian (returns LorentzVector)_datatype_mixinsAttributeError; OneToMany relations typed via the OneToOne key;get_schemasilent NoneWeightStatistics+/+=returned None; delayed multivariation didn't fill missing weights;partial_weightrejected multivariation modifiers; PackedSelection shared mutable cache masks; Up/Down substring corruptionrand_gaussempty-partition IndexError;build()mutated caller layout; broken error messagesnumpy.intin EA parsingmode="first"IndexError + stale site counts; LumiData dtype cast + call-order consistencyWRITABLEmisspelling; triton ms-as-seconds backoff (~53 min sleeps); O(n²) batch concatenate (*meta-inference half deferred — needs live Triton)passgate fails on skipped/cancelled; release needs test-dask-client_map_schema_parquet) deliberately untouched (enabled by an upcoming PR)Dead-code deletions and the public-API line. Because coffea is a library (and
nanoevents.transformsfunctions are reachable via string tokens in user-emitted form keys, invisible to any in-repo grep), each #1578 dead-code item was triaged against external usage (GitHub code search), not just internal references:analysis_tools._generate_slices,util._hex/_ascii, 4 provably-unreachableNotImplementedErrorbranches.flatten_idxs(usesnumpy.int, crashes on numpy ≥ 1.24), the internally-brokenbegin_end_mapping_with_xyzrecordfamily (its own_formemitter dispatches to a different token).dense_evaluated_lookup(+ its evaluator registry entry; user-maintained converters emit its type string) andtransforms.nested_local2global(referenced by external custom schemas) are restored unchanged.tailtimeoutonFuturesExecutor/ParslExecutor— documented, passed by real analyses, but never had an effect; it remains an accepted no-op field that emitsDeprecationWarningwhen set, for removal in a later release.Verification: every behavioral fix has a test confirmed to FAIL on unfixed master with the expected error (40 new test functions / 53 cases). Full suite on this branch: 1171 passed / 48 skipped (optional deps) / 3 xfail; the only 8 failures are the pre-existing FCC serial-run baseline (conftest dask-client default-scheduler leak — they pass standalone; fixed by #1593).
pre-commit run --all-filesclean.Deliberately out of scope: 16 (PDUNE — needs a use/deprecate decision), 23 (eager future cancellation — design change, recommend opt-in), 18/36/74 (open pydantic PRs), 64 (upcoming parquet PR).
🤖 Generated with Claude Code