Vector updates - #272
Vector updates#272
Conversation
Conflicts were limited to pyproject.toml and uv.lock
|
@cophus just pinging you as discussed so we can get this into dev :) |
Copying an empty fixed-grid selection such as v[[], :] raised "Shape dimensions must be positive" because rebuilding the result round-tripped its (0, n) shape through validate_shape. Zero-length axes are reachable through indexing, so accept them and reject only negative dimensions. torch.cat/torch.stack take a sequence of tensors, so Vectors inside that sequence were never seen as arguments and torch reported an opaque "Multiple dispatch failed" error. Detect them and explain that flatten() is the way to combine ragged rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…idy docstrings - set_flattened, field assignment and in-place arithmetic now write all selected rows with one index_put_ instead of a per-cell Python loop (256x256 grid: set_flattened 291 -> 2 ms, += 503 -> 6 ms). - In-place ops that change dtype kind (int Vector /= 2) raise a TypeError instead of torch's index_put dtype error. - tensor + vector (and t.add(vector), etc.) return a Vector, matching vector + tensor. - from_data accepts [] for empty cells, and [[], []] is two empty cells. - Docstrings: add Parameters/Returns to public constructors and mutators, remove em dashes and emphasis, fix RST list formatting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # uv.lock
cophus
left a comment
There was a problem hiding this comment.
I pushed two commits to this branch.
41aeffc (fixes and docstrings)
set_flattened, field assignment and in-place arithmetic now write all selected rows with a singleindex_put_instead of looping over cells. On a 256×256 grid with 5–30 rows per cell,set_flatteneddrops from 291 ms to 2 ms, and+=from 503 ms to 6 ms.- In-place operations that change dtype kind, such as
/=on an integer Vector, now raise a clearTypeErrorinstead of the torchindex_putdtype error. tensor + v(andt.add(v), etc.) now returns a Vector, matchingv + tensor.from_dataaccepts[]for an empty cell, and[[], []]is now read as two empty cells rather than one cell with two zero-length rows. This bug predates the torch migration, and it matters for Bragg peak data, where many cells are empty.- Public constructors and mutators now have Parameters and Returns sections, and the RST list formatting is fixed.
a78e0e5 merges dev and resolves the uv.lock conflict. I regenerated the lock from dev's version, so it differs from dev only by polars.
I also left five inline comments with specific changes. I tested each one against a78e0e5, and all Vector tests pass:
- NumPy operands promote to float64 (
_binary_op): cast non-tensor operands to the Vector dtype, as the in-place path already does. - Single-cell indexing (
_selected_cell_indices): cache the cell index range and add an all-integer fast path. 1000 calls ofv[i, j]drop from 55 ms to 2.8 ms (NumPy version: 13 ms). from_data(from_data): build the row buffer with one concatenation. 284 ms drops to 36 ms (NumPy version: 52 ms).- 1D operands on single-field Vectors (
__torch_function__): makex + vapply one value per row, matchingv + x. - Stale views after
rename_fields: document the behavior and add a test for now, and decide on a fix separately.
Items 1 to 4 are ready as code. If you agree with them, I'll push them with tests, or you can apply the suggestion blocks directly.
| @@ -406,7 +558,11 @@ def rename_fields(self, mapping: dict[str, str]) -> None: | |||
| self._selected_fields = tuple(rename.get(f, f) for f in self._selected_fields) | |||
There was a problem hiding this comment.
Views created before a rename keep the old field names, so kx = v.select_fields("a"); v.rename_fields({"a": "x"}); kx.flatten() raises KeyError. Fixing this properly means views would store column indices, which would change how add_fields and remove_fields update views. For this PR, please document it in the rename_fields docstring:
Notes
Views created before the rename keep the old field names and raise
KeyError; create new views with select_fields after renaming.
and add a test that asserts the KeyError, so a later fix has to update it.
| elif _is_scalar(other): | ||
| rhs = _scalar_value(other) | ||
| else: | ||
| rhs = _broadcast_field_values( |
There was a problem hiding this comment.
v * np.ones(3) returns a float64 Vector, because NumPy arrays default to float64 and torch promotes float32 * float64 to float64. In a NumPy workflow almost every operand array is float64, so arithmetic silently doubles memory use and changes dtype. The in-place path (v *= np.ones(3)) keeps float32, so the two paths currently disagree. Please cast non-tensor operands (NumPy arrays, lists) to the Vector's dtype, and keep torch promotion for tensor operands:
dtype=None if isinstance(other, torch.Tensor) else lhs.dtype,
Tested: (v * np.ones(3)).dtype is float32, (v * torch.ones(3, dtype=torch.float64)).dtype is still float64, and all tests pass. Please also add a test asserting both dtypes.
| """Return linear cell indices for the current fixed-grid selection.""" | ||
| if self._selection_indices is None: | ||
| return np.arange(_cell_count(self._state["shape"]), dtype=np.int64) | ||
| return torch.arange(_cell_count(self._state["shape"]), dtype=torch.int64) |
There was a problem hiding this comment.
v[i, j] on a root Vector rebuilds torch.arange(num_cells) on every call. On a 256×256 grid this accounts for 87% of the time per access (55 µs, compared with 13 µs for the NumPy version). Please cache it:
return _cell_arange(_cell_count(self._state["shape"]))
and add next to _cell_count (with import functools):
@functools.lru_cache(maxsize=8)
def _cell_arange(num_cells: int) -> torch.Tensor:
"""Cached arange(num_cells), shared read-only by every root Vector of that size."""
return torch.arange(num_cells, dtype=torch.int64)
Please also add an all-integer fast path at the top of _select_linear_indices. It should .clone() the result so views never share memory with the cache:
key = idx if isinstance(idx, tuple) else (idx,)
if len(key) == len(shape) and all(type(i) is int for i in key):
linear = 0
for i, size in zip(key, shape):
if not -size <= i < size:
raise IndexError("Vector index out of range")
linear = linear * size + (i % size)
return (), current_indices[linear : linear + 1].clone()
Tested: 1000 accesses take 2.8 ms instead of 55 ms. Negative and out-of-range indices behave as before.
| _token=cls._token, | ||
| ) | ||
| vector._replace_cells(np.arange(len(cell_arrays), dtype=np.int64), cell_arrays) | ||
| vector._replace_cells(torch.arange(len(cell_arrays), dtype=torch.int64), cell_arrays) |
There was a problem hiding this comment.
from_data takes 284 ms on a 256×256 grid, compared with 52 ms for the NumPy version, because each cell goes through its own as_tensor, reshape and dtype cast. Please build the buffer directly:
num_fields = vector._full_num_fields
lengths = torch.tensor([array.shape[0] for array in cell_arrays], dtype=torch.int64)
rows = [array.reshape(-1, num_fields) for array in cell_arrays if array.shape[0] > 0]
if rows and all(isinstance(array, np.ndarray) for array in rows):
vector._state["data"] = vector._to_buffer(_as_tensor(np.concatenate(rows)))
elif rows:
vector._state["data"] = vector._to_buffer(
torch.cat([_as_tensor(array) for array in rows], dim=0)
)
vector._state["cell_lengths"] = lengths
vector._state["cell_starts"] = torch.cumsum(lengths, 0) - lengths
For this to help, _coerce_inferred_cell_array also needs to return NumPy input unchanged (reshaped to 2D) instead of calling _as_tensor. Tested: 36 ms. The output buffer is identical to the current version, and mixed NumPy/torch/list leaves still work.
|
|
||
| def __truediv__(self, other: Any) -> "Vector": | ||
| return self._binary_op(other, np.divide) | ||
| return self._binary_op(other, torch.divide) |
There was a problem hiding this comment.
For a single-field Vector, v + x with a 1D x of length total_rows adds one value per row. x + v instead uses torch broadcasting and returns a (total_rows, total_rows) tensor. Please apply the same per-row rule in torch dispatch:
# Single-field Vectors read a 1D tensor as one value per row, as in v + x.
per_row = sum(row_counts) if template.num_fields == 1 else None
flat_args = tuple(_flatten_torch_input(value, per_row) for value in args)
flat_kwargs = {key: _flatten_torch_input(value, per_row) for key, value in kwargs.items()}
and in _flatten_torch_input(value, per_row=None), add
if isinstance(value, torch.Tensor) and value.ndim == 1 and value.shape[0] == per_row: return value.reshape(-1, 1).
Restricting this to single-field Vectors matters: for a multi-field Vector with total_rows == num_fields, the reshape would make x + v and v + x disagree. Tested: both orders now give the same result for single-field and multi-field Vectors. Please add a test with x = torch.arange(total_rows).
…, 1D operands - NumPy and list operands are cast to the Vector dtype, so v * np.ones(3) stays float32; tensor operands keep torch type promotion. - Cache the root cell index range and add an all-integer fast path to fixed-grid indexing (1000 v[i, j] calls: 55 ms -> 3 ms on 256x256). - from_data joins NumPy cells with one np.concatenate and converts once (256x256 grid: 284 ms -> 36 ms). - For single-field Vectors, a 1D tensor of length total_rows is applied one value per row in both x + v and v + x. - Document that views created before rename_fields keep stale field names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- NumPy and list operands promote like Python scalars, so a float32 Vector stays float32 and an integer Vector times float values is no longer truncated to zero. - The per-row reading of 1D tensors applies only to elementwise functions, so torch.index_select and torch.matmul keep their usual 1D semantics. - from_data casts torch cells to the buffer device before concatenating. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # uv.lock
What problem this PR addreseses
Move Vector to torch, add to_polars()
Vector is only used by a few folks, so it seemed easiest to just break things by moving to torch rather than doing a slow migration.
Breaking changes
New
Bug fix
select_fields("c","b","a") returned data in storage order while .fields reported the requested order. Pre-existing (same logic in the NumPy version); fixed and tested.
Serialization
save() writes the row buffer as compressed Zarr rather than an uncompressed torch.save blob, and a new _post_load hook on AutoSerialize rehydrates to CPU tensors. Consequences: pre-migration files still load (keeping their original dtype), and a GPU-saved Vector loads onto CPU.
Known limitation: this only applies when the Vector is the save root. AutoSerialize walks nested objects via _recursive_save, so a Vector held as an attribute of another serializable object still round-trips correctly but writes uncompressed (~48% larger). The real fix is teaching _serialize_value to store plain non-grad tensors as Zarr arrays — which would also fix tensor-backed Dataset, currently saving fully uncompressed. Documented in save(), deliberately out of scope here.
What should the reviewer(s) do
Make sure it works for your workflows, make fixes.
Take a look at the tutorial: https://github.com/electronmicroscopy/quantem-tutorials/blob/main/tutorials/core/vector.ipynb