Repository navigation
Speed up voxel lookup, and reject an unordered lookup instead of trusting it - #34
Conversation
`_matching_voxel_indices` asked `np.isin` whether each query voxel was present, then found it with `np.searchsorted`. `np.isin` walks and internally sorts the whole lookup column, so the cost was O(rows) no matter how few voxels were queried -- 61.9 million rows for the real `closest surface voxel` table, paid on every call. `angle.find_closest_streamline` calls the helper with a single voxel, so one coordinate cost a full pass over the table: 12.5 s measured against the real file. `np.searchsorted` already returns the left-hand insertion point, so the query is present exactly when the key at that row equals it. Reading membership off that search gives the same matches, the same `missing_value`, and the same tie-breaking, in O(queries * log rows): the same single-voxel lookup now takes 0.046 ms, and 40k mixed queries drop from 13.5 s to 105 ms. Verified against the real reference files: outputs are identical for the 61.9M-row closest-surface-voxel lookup and for all nine view lookups through the `sorter` path, including `flatmap_butterfly` with its 350,956 tied keys. The helper already assumed the lookup column was ordered -- that is what `searchsorted` requires -- and the docstring now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`np.searchsorted` requires non-decreasing keys and does not verify it, so a reference file whose lookup column was out of order produced wrong voxel matches with no error -- the failure mode CLAUDE.md flagged as a gotcha. `_check_lookup_is_ordered` now runs inside `_matching_voxel_indices` and raises `ValueError` naming the first out-of-order entry, so a bad file can be located rather than merely rejected. It covers both orderings the helper searches: the plain column, and the column as reordered by `sorter`. Where the check runs is the whole design problem. The scan is O(rows) -- 74 ms on the real 61.9M-row closest-surface-voxel table, against 0.03 ms for the lookup itself -- and `angle.find_closest_streamline` looks up a single voxel per call, so checking on every call would reinstate exactly the per-coordinate O(rows) cost the previous commit removed. The answer is therefore memoised per array in a `WeakValueDictionary`: values are weak, so an entry dies with its array and an `id` cannot be reused while its entry is live. Measured with the check in place: the first lookup against the real table pays 74 ms, every later one 0.03 ms; `find_closest_streamline` over 10 coordinates 1.45 s -> 1.76 s (the one-time scan); `project_coordinates` over 300 coordinates unchanged at ~1.2 s, the sorter-path check being free next to the `argsort` already there. A deliberately corrupted 61.9M-row lookup is rejected, naming the row. The trade-off the cache buys is that a lookup reordered in place after its first successful use is not re-checked. Reference files are read once and left alone; the test pins the behaviour so the cache is not dropped without restoring the per-call cost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up: the unordered lookup now raisesPushed
It covers both orderings the helper searches — the plain column, and the column Where the check runs is the design problemThe scan is O(rows): 74 ms on the real 61.9M-row table, against 0.03 ms So the result is memoised per array in a module-level
The The trade-off, stated plainlyA lookup array reordered in place after its first successful use is not
|
Two related changes to
_matching_voxel_indices, the shared voxel-matchinghelper: it stops paying a cost proportional to the size of the lookup, and it
stops trusting the ordering it depends on.
1. Resolve membership from the binary search, not a full-column scan
The helper answered two questions with two operations:
np.isinwalks and internally sorts the entire key column, so the call costO(rows)regardless of how few voxels were queried. The realclosest_surface_voxel_lookup.h5holds 61,911,881 rows, andangle.find_closest_streamlinecalls the helper with a single voxel — so onecoordinate paid for a full pass over the whole table.
np.searchsortedalready returns the left-hand insertion point, so a query ispresent exactly when the key sitting at that row equals it. Membership is now
read off the search the helper already performs, in
O(queries · log rows).Same matches, same
missing_value, same tie-breaking (still the left insertionpoint, so the stable sorter
IsocortexCoordinateProjectorpasses for issue #12keeps doing what it did).
find_closest_streamline, 10 coordinatesproject_coordinates, 300 coordinatesOutputs are identical, old vs new, on:
closest surface voxellookup (40k queries mixing presentkeys, absent keys, and both out-of-range ends);
sorterpath, includingflatmap_butterflywith its 350,956 tied keys, which is where tie-breakingwould show up if it had changed;
angle.find_closest_streamlineandIsocortexCoordinateProjector.project_coordinatesend to end on real files.Plus 1,600 randomized small-array comparisons against the previous
implementation (duplicate keys, out-of-range queries, both
missing_values,with and without a
sorter).2. Reject an unordered lookup
searchsortedrequires non-decreasing keys and does not verify it, so areference file whose lookup column was out of order produced wrong voxel matches
with no error — the failure mode CLAUDE.md carried as a gotcha, and which
test_without_a_sorter_an_unsorted_lookup_returns_wrong_answers_silentlycharacterized.
_check_lookup_is_orderednow raisesValueErrornaming the first out-of-orderentry, so a bad file can be located rather than merely rejected:
It covers both orderings the helper searches — the plain column, and the column
as reordered by
sorter.Where the check runs is the design problem
The scan is
O(rows): 74 ms on the real 61.9M-row table, against 0.03 msfor the lookup itself.
find_closest_streamlinelooks up a single voxel percall, so checking on every call would reinstate precisely the per-coordinate
O(rows)cost part 1 removes — the check would undo the fix.So the result is memoised per array in a module-level
WeakValueDictionary.Values are weak, so an entry dies with its array and an
idcannot be reusedwhile its entry is live.
find_closest_streamline, 10 coordsproject_coordinates, 300 coordsThe
sorterpath is not memoised and does not need to be:_calculate_2d_coordinatesbuilds its sorter withnp.argsorton every call, soan
O(rows)check is strictly cheaper than what is already there.The trade-off, stated plainly
A lookup array reordered in place after its first successful use is not
re-checked. Reference files are read once and left alone, so this is a fair
trade — but
test_the_ordering_check_is_remembered_per_arraypins it, so thecache cannot be dropped without someone noticing they have restored a
per-coordinate
O(rows)cost. If you would rather have unconditional checking,deleting the cache lookup is a one-line change that costs ~74 ms per coordinate
in
find_closest_streamline.Notes for review
searchsortedneeds — but only the caller-facing gotcha said so. It is nowthe docstring and an enforced precondition.
test_the_key_column_is_never_scanned_end_to_endforbidsnp.isin/np.in1dvia monkeypatch as a stand-in for the
O(rows)cost; a timing assertion wouldmake the same point far less reliably.
uv run pytest: 366 passed. Real-data tier: 36 passed.🤖 Generated with Claude Code