Skip to content

[Perf] Tiles 4d: Consolidate slice error tests using parametrize#508

Merged
hughperkins merged 35 commits into
mainfrom
hp/tiles-4d
Apr 20, 2026
Merged

[Perf] Tiles 4d: Consolidate slice error tests using parametrize#508
hughperkins merged 35 commits into
mainfrom
hp/tiles-4d

Conversation

@hughperkins
Copy link
Copy Markdown
Collaborator

@hughperkins hughperkins commented Apr 17, 2026

Issue: #

Brief Summary

Summary

Consolidates 7 individual slice-error tests into 3 parametrized tests, reducing boilerplate while adding store-side error coverage that was previously missing.

What's in this PR

  • test_tile16_load_slice_errors — replaces test_tile16_load_negative_row_raises, test_tile16_load_negative_col_raises, test_tile16_load_missing_start_raises,
    test_tile16_load_missing_stop_raises. Parametrized over 4 cases (neg_row, neg_col, no_start, no_stop) using qd.static() branching and bad_slice: qd.Template.
  • test_tile16_store_slice_errors — new parametrized test covering negative-index and missing-start/stop errors on the store side (d[-1:tile_size, ...],
    d[:tile_size, ...], d[0:, ...]). Previously only test_tile16_store_missing_stop_raises existed; this adds 3 new error cases.
  • test_tile16_vec_slice_errors — replaces test_tile16_vec_slice_missing_stop_raises and test_tile16_vec_slice_missing_start_raises. Parametrized over 2 cases.
    Net change: 89 lines removed, 50 lines added.

Good

  • Reduces 7 nearly-identical test functions to 3, eliminating duplicated kernel boilerplate
  • Adds store-side error coverage for negative indices and missing start that didn't exist before
  • Uses bad_slice: qd.Template as an explicit kernel parameter (consistent with fastcache=True purity)

Bad

  • The qd.static() branching pattern in the error tests is less immediately readable than separate test functions — you have to trace the parametrize values to understand
    what each case tests
  • All 4 load-error and 4 store-error cases compile into a single kernel with dead branches, which is slightly more work for the compiler (negligible in practice since these
    are error tests)

copilot:summary

Walkthrough

copilot:walkthrough

The purity checker was special-casing qd.math via a local import.
Replace with a general is_from_quadrants_module() check that covers
any quadrants module or class (but not instances, to avoid suppressing
mutable state), and remove the qd_math special case.

Add tests for both the allowed case (qd.math.pi) and the rejected case
(non-quadrants object attribute).
Use == 'quadrants' or .startswith('quadrants.') instead of bare
.startswith('quadrants') to avoid matching hypothetical third-party
packages like quadrantslib.
The merge resolution accidentally dropped this import, which is still
used in build_Name. Restores the import alongside is_from_quadrants_module.
Parametrize 16 existing tile16 tests with [qd.ndarray, qd.field] to
verify both tensor backends produce identical results. Uses an _ann()
helper to select the correct kernel annotation per tensor type.
Pass _TILE as a qd.Template parameter (tile_size) to all 31 kernels,
eliminating closure captures that trigger purity violations. All
captured fields use qd.Template annotations, captured ints use qd.i32.
Removes test_tile16_size_constant_in_kernel (needs public Tile16x16 API).
These were closure-captured and used in qd.static() blocks, which would
break fastcache since the cache key wouldn't vary with their values.
Adds the _Tile16x16Proxy class and qd.simt.Tile16x16 lazy accessor,
enabling qd.simt.Tile16x16.zeros(dtype=...) and .eye(dtype=...) inside
kernels. Includes 18 new tests covering the proxy, vec-proxy patterns,
potrf, trsm, partial load/store, f64 roundtrip, CPU error, and reinit
dtype survival. SharedArray slice tests are intentionally excluded.
GPU clock cycle ratios can deviate >1 from the ideal linear scaling,
especially under CI load. Previous run showed a[12]/a[0] ≈ 11.97
vs expected 13, a deviation of 1.03 that just exceeded the ±1 bound.

Made-with: Cursor
Replace _make_tile16x16/Tile() pattern with qd.simt.Tile16x16.zeros()
and qd.simt.Tile16x16.eye() across all tests except
test_tile16_make_caching and test_tile16_solve_triangular_upper_raises.
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — mechanical test consolidation using , no logic changes.

Extended reasoning...

Overview

This PR modifies a single test file (), consolidating 7 individual slice-error test functions into 3 parametrized tests (, , ). The refactor uses branches inside the kernel to select the bad-slice variant at template-instantiation time, matching the pattern already established elsewhere in the file.

Security Risks

None. This is a test-only change with no production code modifications.

Level of Scrutiny

Low. The change is purely mechanical — same assertions, same error messages, same kernel structure — just deduplicated via parametrize. The parametrize tuples cover the same four cases (neg_row, neg_col, no_start, no_stop) as the original individual tests.

Other Factors

No bugs reported by the automated bug hunter. No prior reviews. The pattern (parametrized kernel with + ) is already used extensively in the file, so this is a consistent, low-risk change.

Was hardcoded to _ATOLS[qd.f32] despite being parametrized over
qd_dtype, making the f64 path 100,000x too lenient.
Copy link
Copy Markdown
Collaborator

@erizmr erizmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on the basis that I have reviewed the design and the public facing API, tests, and they look reasonable to me.

Base automatically changed from hp/tiles-4c to main April 20, 2026 06:02
@hughperkins hughperkins enabled auto-merge (squash) April 20, 2026 06:03
Made-with: Cursor

# Conflicts:
#	tests/python/test_tile16.py
@hughperkins hughperkins merged commit 0c5f2d9 into main Apr 20, 2026
47 checks passed
@hughperkins hughperkins deleted the hp/tiles-4d branch April 20, 2026 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants