fix(nanoevents): FsrPhoton.matched_muon resolves against Muon (not Jet) in dask mode

[NJManganelli]

Part of #1578 — critical bug 2: FsrPhoton.matched_muon resolves against wrong type in Dask.

FsrPhoton.matched_muon had a copy-paste bug in its dask code path: it called Jet._apply_global_index instead of Muon._apply_global_index. As a result, in dask mode the property silently returned Jet records — indexed by FsrPhoton_muonIdx — carrying Jet fields and values, whereas the eager and virtual paths correctly resolved against the Muon collection. No error is raised, so downstream analyses using FsrPhoton.matched_muon under Dask read the wrong physics objects.

The fix points the dask path at the Muon collection to match the eager path. A regression test (test_fsrphoton_matched_muon_type, parametrized over eager/virtual/dask) asserts that matched_muon has Muon-type fields and values matching the eager result across all three execution modes; it fails on master (dask variant) and passes with this fix. Full tests/test_nanoevents.py passes and pre-commit is clean.

🤖 Generated with Claude Code

[lgray]

By-eye review is clear, running the bot to check for knock on effects and any other less-clear problems.

[lgray]

🤖 AI text below 🤖

Summary: The one-line fix is correct and complete. On master, FsrPhoton.matched_muon's dask branch called Jet._apply_global_index(dask_array.muonIdxG) and silently returned Jet records indexed by FsrPhoton_muonIdx — I reproduced this: with the test in place and only nanoaod.py reverted to master, the dask variant fails with Jet fields (area, btagDeepB, ...) where Muon fields belong, with no exception raised; eager and virtual pass (virtual rides the eager code path, so it was never affected). The fixed line (src/coffea/nanoevents/methods/nanoaod.py:547) now mirrors the eager path (:543) and matches the sibling matched_* pattern exactly (e.g. Muon.matched_jet :399, Jet.matched_muons :616). I also swept every _apply_global_index call pair in the file: this was the only eager/dask collection mismatch — the fix class is complete. At the PR head, the new test passes in all three modes and the full tests/test_nanoevents.py passes (21 passed, 1 network test deselected). Recommend merging after one small test improvement below.

Should fix

• tests/test_nanoevents.py:159 — the value assertion is vacuous on this sample: nano_dy.root contains zero FsrPhotons across its 40 events (measured via nFsrPhoton), so ak.all(...) over the empty comparison is trivially True regardless of which collection is resolved, and the "Values must match the eager result" comment overstates what's exercised. The field-set assertions (:154–157) are form-level, so the test does discriminate as-is (proven by the revert run) — but switching the sample to nano_dimuon.root (already used by neighboring tests) makes the value check real: it has 1 FsrPhoton with muonIdx=0, and the matched muon pt (22.46) differs from the Jet pt at the same index (28.58). One-line change (nano_dy.root → nano_dimuon.root at :132).

Nits/Optional

• The full field-set equality at :154 already subsumes the two spot checks at :156–157 and would also catch a swap to any other collection (e.g. Electron, which lacks pfRelIso04_all) — the spot checks are fine to keep as readable documentation of the failure mode.
• Out of scope for this PR, but the sweep surfaced two pre-existing oddities in the same file worth a follow-up under #1578: nanoaod.py:828 (AssociatedPFCand.jet dask path calls dask_array.events() — no such method on arrays, only the factory has events(), so this raises AttributeError) and nanoaod.py:860–866 (AssociatedSV.jet indexes self._events()[collection] where collection is already an array). Both fail loudly and are PFNano-only, unlike the silent bug fixed here.

Test coverage: The regression test is non-vacuous and discriminating — verified by reverting only nanoaod.py to master and observing the dask variant fail at the field-set assertion while eager/virtual pass, then restoring the fix and observing 3/3 pass. It covers all three execution modes, checks field membership against the eager reference, and reuses the tests_directory fixture, local sample files, and the pytest.importorskip("dask_awkward") convention consistently with neighboring tests. The one weakness is the vacuous value assertion noted above.

Pydantic: Not applicable — the diff touches no pydantic code (0 grep matches).

[lgray]

Actually a good catch - @NJManganelli do we have any nanoevents samples with actual FSRPhotons in them? Probably just need to skim some out of a Zmumu nanoaod sample.

[ikrommyd]

What I was seeing for example regarding excessive testing from LLMs is that I think for example this PR does not need a single test. It's a one-line fix. It's not like we're going to accidentally revert that such that we need a regression test.

[lgray]

And what if we do and miss it (again)?

[ikrommyd]

I'd hope that we are not stupid enough to revert this. What I think would be way more valuable is a test that loops over all of those _apply_global_index methods for every schema and makes sure they all have the right output type. The current test just hardcodes fsrphoton.

I'm not saying I want any changes here, just pointing out what I meant when I said that I don't like how LLMs often add useless tests that just add lines of code. Just my personal opinion.

[lgray]

I hope we are not that stupid either, but I also don't bet against stupidity winning eventually. It really tends to.

[NJManganelli]

I'd say the right thing to do is actually test intended mapping more generally: matched_X should probably test that e.g. the index being fed in has some relation to the object you match against; testing we don't revert this one bug is kinda useless, but testing we match intended cross-references? Genuinely useful. Without looking at branch names, I expect that's trivial.

For the FSR Photons, maybe the nanov15 test files that Alejandro added in one of the PRs for JECs or Type1-MET?

[nsmith-]

I agree with @ikrommyd, DRY and YAGNI principles should apply to tests too

[lgray]

I agree with those principles too, however, we (apparently) needed it. 🤷

[NJManganelli]

Reworked the test per the discussion here (thanks @ikrommyd / @nsmith- / @lgray). Rather than pin this one line, the PR now carries two complementary, low-maintenance tests:

1. Mode-consistency sweep — auto-discovers every _apply_global_index-backed cross-reference straight from nanoevents.methods.nanoaod (32 found; 22 exercised on nano_dy, 10 skipped as absent from that sample) and requires eager/virtual/dask to agree on the resolved record type and fields. New cross-refs are covered automatically. This is the general "all _apply_global_index methods return the right output type" check @ikrommyd suggested, and it catches the whole copy-paste-owner class rather than just this bug.

2. Absolute declared-target check (static, no sample) — the sweep above is differential, so it can't see a cross-ref wired to the wrong collection in every mode (e.g. Muon.matched_jet → Electron). This companion parses each cross-ref's eager and dask source bodies and asserts the _apply_global_index owner matches a hand-declared target table, with completeness assertions binding the table to auto-discovery (you can't add a cross-ref without declaring its target, and stale entries fail). It flags the FsrPhoton bug statically — resolves against Jet, expected Muon — and would catch the wrong-collection case too.

On the "do we even need a test" thread: I think this lands where YAGNI/DRY and @lgray's "what if we revert and miss it" both want to be — no one-off regression, but real coverage of the mechanism.

Re @lgray's sample request: #1537 (@alefisico) is adding nano_tt_v15.root, which does contain populated FSRPhotons (12 across 10 events, real FsrPhoton_muonIdx). Once that merges we can harden the sweep from type-level to a value-level check (matched muon kinematics == Muon[muonIdx]) without skimming a new file. (Aside: v15 also adds FsrPhoton_electronIdx, so a future FsrPhoton.matched_electron may be worth a separate issue.)

[NJManganelli]

This is ready from my end, I'd like another set of human eyes to pass over it at least. Some of the test comments are still a bit verbose, but it's not too egregious and the text is sufficiently useful to keep IMO.
@lgray @nsmith- @ikrommyd