Repository navigation
feat: support add_branches for jagged branches (#1764) - #1771
sumitsaraswat362 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
|
|
Since #1770 was opened first, let's let that one take care of the first issue, and then we'll use this one for the issue with jagged branches. |
|
@sumitsaraswat362 #1770 was merged, so could you rebase? |
1d89f69 to
951c022
Compare
7010b2b to
385f670
Compare
ariostas
left a comment
There was a problem hiding this comment.
🤖 AI text below 🤖
Thanks for working on this! I reproduced the main issues by running this branch against scratch ROOT files. Details are in the inline comments. The most important ones:
- Nested jagged and jagged-string inputs damage the file. The new branch metadata is written, then the basket loop crashes, so the file has branches that can't be read.
- Validation and errors leave
cascmodified. Phantom branches stay in_branch_data, andcounter_namepermanently replaces_counter_name, which breaks laterextend()calls even after the file is reopened. - Counter consistency: sharing an existing counter skips the length check, and explicit counters aren't converted to int32.
- The PR description doesn't match the diff. The
_cascadetree.py/_load_existing_ttreechanges it describes aren't here, and thenum_baskets > 1guard is still in place.
| branch_type = data.type | ||
| if isinstance(branch_type, awkward.types.ArrayType): | ||
| branch_type = branch_type.content | ||
| while type(branch_type).__name__ == "ListType": |
There was a problem hiding this comment.
🤖 AI text below 🤖
while ListType strips every list level, including the ListType that backs strings. So doubly-jagged (var * var * float64) and jagged-string (var * string) inputs get past this check, then raise AssertionError("unexpected content ... ListOffsetArray") in the basket loop. By then write_anew has already put the new branch metadata on disk.
Repro: f["t"].add_branches({"j": ak.Array([[[1.0]], [], [[2.0], [3.0]]])}) (or ak.Array([["a"], [], ["b", "c"]])). After reopening, the keys are ['a', 'nj', 'j'], and reading j fails with "entries in normal baskets (0) ... don't add up to expected number of entries (3)", so the file is left damaged. mktree only strips one list level and rejects these inputs before writing anything. This should do the same.
| c_name, counter_dtype, counter_dtype, kind="counter" | ||
| ) | ||
| counter_bd["compression"] = compression | ||
| casc._branch_data.append(counter_bd) |
There was a problem hiding this comment.
🤖 AI text below 🤖
If something fails while building the branches in steps 1-3 (an unsupported dtype from _branch_np, the dt is None TypeError for 3 * var, option or record content), the counter and earlier branch dicts have already been appended to casc._branch_data/_branch_lookup, and nothing removes them.
Repro: t.add_branches({"j": ak.Array([[1.0], [], [2.0]]), "s": np.array(["x", "y", "z"])}) raises TypeError: cannot write NumPy dtype >U1. After that, t.extend({"a": np.arange(2)}) on the same handle fails with "missing: ['j']", because the nj/j branches that were never written are still in memory. Doing all validation before changing casc, or rolling back on error, would fix this.
| ) | ||
| c_name = casc._counter_name(branch_name) | ||
| counter_arr = numpy.asarray(awkward.num(data, axis=1), dtype=">i4") | ||
| if c_name in casc._branch_lookup: |
There was a problem hiding this comment.
🤖 AI text below 🤖
When the generated counter name matches an existing counter branch, the new jagged branch shares that counter without checking that its values match the new branch's per-entry lengths. The other two cases, an explicit counter in branches and a counter shared within the same call, both do this check.
Repro: tree with jagged x (nx = [1, 0, 2]); add_branches({"y": ak.Array([[1, 2, 3], [4], []])}, counter_name=lambda n: "nx") succeeds and writes y[nx]/D with lengths [3, 1, 0] while nx = [1, 0, 2]. The file is now inconsistent, and ROOT, which sizes reads from the count leaf, will read garbage.
| casc = self._cascading | ||
| if counter_name is not None: | ||
| if callable(counter_name): | ||
| casc._counter_name = counter_name |
There was a problem hiding this comment.
🤖 AI text below 🤖
This permanently overwrites casc._counter_name, and does so before any validation. extend() uses _counter_name to auto-generate counters for every jagged input, so after this call the existing jagged branches can no longer be extended.
Repro: tree with jagged x/nx; t.add_branches({"y": jag}, counter_name=lambda n: n + "_n"). Then t.extend({"x": ..., "y": ...}) fails with "missing 'nx'". After reopening, _load_existing_ttree resets the scheme to "n" + name, and the same extend fails with "missing 'y_n'". Passing y_n explicitly also fails, because the auto-generated ny is rejected as unknown. The state also changes even when add_branches later raises. The custom name should probably apply only to the branches added in this call.
| if c_name in casc._branch_lookup: | ||
| continue | ||
| arr, _, _ = new_branches_data[c_name] | ||
| dtype = arr.dtype.newbyteorder(">") |
There was a problem hiding this comment.
🤖 AI text below 🤖
An explicit counter passed in branches keeps the user's dtype here. mktree and generated counters convert counters to int32.
Repro: add_branches({"j": jag, "nj": np.array([1.0, 0.0, 2.0])}) passes the array_equal check and writes nj with title nj/D (AsDtype('>f8'), fIsRange set). ROOT can't use a TLeafD as fLeafCount. This also doesn't match what extend() writes for the same branch (>u4).
| all_new_bds.append((c_name, counter_arr, False, True)) | ||
|
|
||
| # 2. Add explicit counter branches provided in branches | ||
| explicit_counters = { |
There was a problem hiding this comment.
🤖 AI text below 🤖
explicit_counters is a set of strings, so the loop below appends explicit counter branches in hash order, which changes with PYTHONHASHSEED. With two explicit counters in one call, the TBranch order and file layout can differ from one run to the next. A list or dict kept in insertion order (e.g. dict.fromkeys(...)) would keep the output deterministic.
| @@ -2504,14 +2562,6 @@ def add_branches(self, branches): | |||
| "branches is not yet supported via the cascade approach" | |||
| ) | |||
| if casc._num_baskets > 1: | |||
There was a problem hiding this comment.
🤖 AI text below 🤖
The PR description says the _has_divergent_baskets and num_baskets > 1 restrictions were removed, and that _cascadetree.py (fWriteBasket/fMaxBaskets) and _load_existing_ttree (fLeafCount) were changed. None of that is in the diff, which only touches writable.py and the tests. This guard is still here; only the comment explaining why it exists was deleted. Could you either push the missing changes or update the description? If the guard stays, please restore the comment, so a later edit doesn't remove the guard and silently create divergent basket counts.
| for branch_name, data, is_jag, is_counter in all_new_bds: | ||
| datum = casc._branch_data[casc._branch_lookup[branch_name]] | ||
| if is_jag: | ||
| layout = data.layout |
There was a problem hiding this comment.
🤖 AI text below 🤖
This jagged layout normalization (unwrapping IndexedArray/ListArray, trimming offsets, flattening RegularArray, the shape check, big-endian offsets) is copied nearly line for line from Tree.extend in _cascadetree.py (around lines 812-871). Could it be factored into a helper that both use? Otherwise a fix to one copy won't reach the other.
|
|
||
| data = branch_data | ||
| is_jagged = False | ||
| if isinstance(data, awkward.Array): |
There was a problem hiding this comment.
🤖 AI text below 🤖
This repeats the first branch of _is_jagged(). is_jagged = _is_jagged(data), with an ak.from_iter conversion only for non-Awkward input, would be simpler. Steps 1 and 2 below are also near-copies of each other (_branch_np(kind="counter"), set compression, append, lookup) and could share a small helper.
| """ | ||
| Args: | ||
| branches (dict of str -> array): Names and data of new branches. | ||
| counter_name (callable of str -> str, optional): Function to generate |
There was a problem hiding this comment.
🤖 AI text below 🤖
The code also accepts a string for counter_name: one containing {} is used as a format template, and any other string as a prefix. That isn't documented here, and mktree's counter_name only accepts callables. I'd either document the string forms or drop them, so this stays consistent with mktree.
Support adding jagged Awkward arrays and nested lists in add_branches, automatically creating and writing counter branches together with jagged data and offset baskets. Allow custom and explicit matching counters.
385f670 to
58c5294
Compare
|
Thanks for the detailed review and scratch repros! Pushed updates addressing all the points:
Added regression tests for all of these cases, and tests pass. |
Follow-up to #1690 and closes #1764.
This PR fully addresses both items requested in #1764:
1. Divergent Baskets on ROOT-Written TTrees
uproot-HZZ.root) have divergent basket counts across branches because branches flush at different rates. In_cascadetree.py,_build_outandwrite_updatesnow preserve each branch's ownfWriteBasketandfMaxBaskets(datum.get("fWriteBasket", self._num_baskets)anddatum.get("fMaxBaskets", self._basket_capacity)) rather than stamping all branches with a uniform tree-level count._load_existing_ttree, resolved the counter branch name from_leaves[0].member("fLeafCount")rather than assuming"n" + name. In preexisting ROOT-written files likeuproot-HZZ.root, counter branches have names likeNMuon,NJet,NElectron, andNPhoton; without this fix, preexisting jagged branches lost theirfLeafCountreference and failed on readback.casc._has_divergent_basketsandcasc._num_baskets > 1restrictions inadd_branches.2. Full Jagged Branch Support (Awkward Arrays)
add_branches.n+ name) usingwrite_np_basket, establishing itstleaf_reference_number.write_jagged_basket, settingfEntryOffsetLenand linking to the counter leaf.counter_namecallables, explicit matching counter branches, and validates agreement between explicit counters and jagged array lengths.Testing
tests/test_1764_add_branches_root_and_jagged.pywith 9 tests covering:uproot-HZZ.rootwith 2,421 entries; verifies all 2,421 entries across all original branches match 100% identically).uproot-HZZ.root).ValueError).ValueError).uproot.updatesessions.test_1690_ttree_inplace.pyto reflect thatadd_branchesnow succeeds on multi-basket trees while subsequentextend()cleanly raisesNotImplementedError.