[Type] ndarray typing 3: Add cook_dtype calls at C++ boundaries

[hughperkins]

…refactor)

Add cook_dtype() calls at all points where dtype values are passed to C++ code. Make PyQuadrants.default_fp/ip/up into properties that always store DataTypeCxx. Rename shadowed dtype var in create_field_member. All changes are behavioral no-ops with current code, preparing for a future refactor of primitive dtypes into Python classes.

Issue: #

Brief Summary

Summary

Pushes cook_dtype() (the conversion from Python-side dtype objects to the C++ DataTypeCxx representation) closer to its consumers, so that:

1. Default integer/float/unsigned types (default_fp, default_ip, default_up) on PyQuadrants are stored already-cooked. Direct attribute access is preserved via
   @property getter/setter pairs that auto-cook on assignment, so user code like qd.init(default_fp=qd.f64) keeps working.
2. C++-bound APIs (sparse matrix/solver constructors, expr_alloca_shared_array, constant-expression makers, etc.) receive cooked dtypes at the call site rather than relying
   on the C++ binding to coerce — making the Python/C++ boundary explicit and catching un-cooked types earlier.
3. As a side effect, fixes a real shadowing bug in create_field_member where the local checkbit dtype was being assigned to the function parameter dtype, mutating the
   caller-visible variable. Renamed to checkbit_dtype and added a debug-mode regression test.
   This PR is stacked on top of hp/typing-t4-2-ndarray-subscript (which itself stacks hp/typing-t4-1-eval-str); both are merged in.

Changes

cook_dtype boundary work

• python/quadrants/lang/impl.py — PyQuadrants.default_fp / default_ip / default_up converted from raw attributes into properties with auto-cooking setters; backing
  fields (_default_fp etc.) are pre-cooked in __init__. expr_init_shared_array now cooks element_type before passing to the AST builder.
• python/quadrants/lang/expr.py — make_constant_expr cooks dtype once up-front and cooks the default_fp / default_ip / u1 fallbacks. Adds an inline comment
  explaining the strategy (cook normalised input once, then cook only the runtime defaults in each branch).
• python/quadrants/lang/matrix.py — make_matrix empty-vector path now uses cook_dtype(primitive_types.i32) instead of the raw Python type.
• python/quadrants/linalg/sparse_matrix.py, sparse_solver.py — SparseMatrix, SparseMatrixBuilder, and SparseSolver constructors cook dtype once and pass
  the cooked value to the C++ factories.

Bug fix

• python/quadrants/lang/impl.py — In create_field_member's debug-mode branch, the local variable holding the checkbit dtype was named dtype, shadowing the function
  parameter. Renamed to checkbit_dtype. Added documentation comment explaining the rename.

Tests

• tests/python/test_ad_basics_fwd.py — test_dual_field_dtype_preserved_in_debug_mode: regression test for the checkbit shadowing bug, gated on data64 since it uses
  qd.f64.

Merged-in work

• All commits from hp/typing-t4-1-eval-str (get_func_signature helper, eval_str=True support for stringified annotations) and hp/typing-t4-2-ndarray-subscript
  (NDArray[i32] single-arg subscript fix) come along via merge.

Good points

• Real bug fix. The dtype shadowing in create_field_member was silently mutating the wrong variable — caller-visible only in subtle debug-mode AD scenarios, which is
  exactly why it had gone unnoticed. Now caught by an explicit regression test.
• Pushes type coercion to the boundary. cook_dtype calls being scattered across deep call paths makes it hard to tell where Python-side dtype objects can vs. can't
  appear. Cooking at the C++ boundary means downstream code (and types — see the DataTypeCxx annotations on the new properties) can assume cooked types from then on.
• Backwards-compatible API surface. Converting default_fp / default_ip / default_up to properties preserves all existing reads and assignments
  (get_runtime().default_fp, qd.init(default_fp=qd.f64), etc.) — the cooking happens transparently.
• Eliminates duplicate cook_dtype calls. A follow-up commit (215544f15) removes redundant cooks in make_constant_expr now that the up-front normalisation handles
  them.
• Test added for the bug. Not just the refactor — the actual semantics-changing fix has coverage.

Bad / weak points (worth reviewer attention)

• Property conversion is a subtle public-API change. default_fp/ip/up are documented attributes. Code that does runtime.default_fp = some_already_cooked_value is
  fine (cooking is idempotent), but anything that introspects via vars(runtime) or runtime.__dict__["default_fp"] will now get nothing / find _default_fp instead. Risk is
  low but not zero.
• Type annotation says DataTypeCxx, not Any. The new properties annotate the return type as DataTypeCxx but the setter accepts Any. That's correct for the
  cook-on-write pattern, but the asymmetric annotation may confuse type checkers downstream.
• Inconsistent cook-site coverage. Sparse matrix/solver and a few other call sites get the boundary treatment, but a grep would likely turn up other C++ bindings still
  receiving raw Python dtypes. This PR doesn't claim to be exhaustive, but reviewers should weigh whether to expand or leave a TODO.
• Test gating. test_dual_field_dtype_preserved_in_debug_mode requires qd.extension.data64 (added after a CI failure on backends without f64 support — see commit
  dcc12164f). This means the regression won't be exercised on f32-only backends; if the bug ever recurs in a different shape there, the test won't catch it. [fixed]
• Stacked branch. Reviewing against main shows the union of t4-1 + t4-2 + t4-3 changes (~150 lines). Reviewers should look at this branch after t4-2 lands, or use git diff hp/typing-t4-2-ndarray-subscript...HEAD to see only the t4-3-specific delta.
• Noisy commit history. 12 logical commits + 3 merge commits since branching from main. Worth squash/rebase before merge.

copilot:summary

Walkthrough

copilot:walkthrough

[hughperkins]

Opus 4.6 review:

PR Review: hp/typing-t4-3-cook-dtype

Branch: hp/typing-t4-3-cook-dtype (parent: hp/typing-t4-2-ndarray-subscript)

Summary

This PR adds cook_dtype() calls at C++ boundaries (expr, impl, matrix, sparse_matrix, sparse_solver) and refactors default_fp/default_ip/default_up in PyQuadrants into properties with backing _default_* attributes. It is a preparatory refactor for a future PR that will convert dtypes to Python classes; with current DataTypeCxx instances, cook_dtype() is effectively a no-op (identity for DataTypeCxx subclasses).

Issues Found

1. Bug fix (positive finding): create_field_member variable shadowing

The rename from dtype to checkbit_dtype in the adjoint checkbit block fixes a real bug. In the original code:

if prog.config().debug:
    dtype = u8
    if prog.config().arch == _qd_core.vulkan:
        dtype = i32
    x_grad_checkbit.ptr = _qd_core.expr_field(..., cook_dtype(dtype))
# ...
x_dual.ptr = _qd_core.expr_field(x_dual.ptr, dtype)  # BUG: dtype was overwritten to u8/i32!

The inner block overwrote the dtype parameter, so x_dual was incorrectly created with u8 or i32 instead of the field’s actual dtype when prog.config().debug was True. The rename correctly isolates the checkbit dtype.

2. Minor: Redundant cook_dtype in make_constant_expr

In make_constant_expr, when dtype is not None, dtype is already cooked at the top. For float/int branches, constant_dtype is then cooked again. Since cook_dtype is idempotent for DataTypeCxx, this is harmless but redundant. Consider removing the second cook_dtype(constant_dtype) for the dtype is not None path, or leave as-is for consistency and future-proofing.

Correctness

• No-op behavior: cook_dtype() returns dtype unchanged when type(dtype) is a subclass of _qd_core.DataTypeCxx. Current primitives (f32, i32, u1, etc.) are such instances, so behavior is unchanged.
• Property refactor: The default_fp/default_ip/default_up properties correctly preserve read/write behavior. The setters apply cook_dtype(value), so future Python dtype classes can be normalized when assigned.
• set_default_ip: Uses self.default_ip = ip and self.default_up = u32 if ip == i32 else u64, which works with current DataTypeCxx values. A future Python dtype class would need == to compare correctly with i32/i64 for this logic to remain correct.

Edge Cases

• cook_dtype(None): Would raise ValueError. Call sites guard with if dtype is not None where appropriate (e.g. make_constant_expr).
• qd.default_fp = float: cook_dtype(float) returns impl.get_runtime().default_fp, i.e. the current value. Assigning float is effectively a no-op and may be surprising; acceptable for a preparatory refactor.
• SharedArray / expr_init_shared_array: element_type is now cooked before passing to C++. Correct and consistent with other boundaries.

Style

• Consistent use of cook_dtype() at C++ boundaries.
• Clear variable naming (checkbit_dtype, dtype_cxx in sparse modules).
• Imports and formatting are consistent with the rest of the codebase.

Suggestions for Improvement

1. Document the bug fix: Add a brief comment near the checkbit_dtype rename noting that it fixes the previous shadowing of dtype, which caused x_dual to get the wrong dtype in debug mode.
2. Optional: Add a test that exercises create_field_member with debug=True and needs_dual=True to lock in the fix and guard against regressions.
3. Future PR: When introducing Python dtype classes, ensure set_default_ip’s ip == i32 comparison works for both DataTypeCxx and the new Python dtype wrappers (e.g. via cook_dtype(ip) == cook_dtype(i32) or an explicit equality method).

Verdict

The PR is correct and behaves as a no-op for current dtypes. It also fixes a real bug in create_field_member when debug mode is enabled. Tests pass. Ready to merge with optional minor suggestions above.

[hughperkins]

@claude review

[claude]

LGTM — mechanical cook_dtype refactor with a real shadowing bug fix and regression test.

Extended reasoning...

Overview

This PR pushes cook_dtype() calls to the Python/C++ boundary in expr.py, impl.py, matrix.py, sparse_matrix.py, and sparse_solver.py. It also converts PyQuadrants.default_fp/ip/up from raw attributes into property getter/setter pairs that store pre-cooked DataTypeCxx values, fixes a shadowing bug in create_field_member (renaming the inner checkbit dtype so it no longer clobbers the outer dtype parameter), and adds a regression test for the bug. The run_tests.py change disables pytest-timeout to avoid a conflict with pytest_hardtle, matching recent commit 2a885b1 on main.

Security risks

None. This is internal type-handling machinery; no auth, crypto, input validation, or data exposure surface.

Level of scrutiny

Low-to-moderate. The changes are mechanical and individually small, and cook_dtype() is a no-op (identity) for current DataTypeCxx instances, so behavior is preserved. The property conversion is a subtle public-API change (code introspecting vars() or __dict__ would be affected), but typical runtime.default_fp reads and qd.init(default_fp=qd.f64) writes continue to work unchanged.

Other factors

The create_field_member bug fix is a real improvement — the prior code silently overwrote the caller-visible dtype parameter in debug mode, causing x_dual to be created with the checkbit dtype (u8 or i32) instead of the field's actual dtype. The new parametrized test (test_dual_field_dtype_preserved_in_debug_mode) exercises this with x=1.25 -> dual=2.5 so a truncation regression would be immediately caught. A prior reviewer-posted Opus 4.6 review already reached the same ready-to-merge conclusion, and I independently agree.