Skip to content

Conversation

@PierreQuinton
Copy link
Contributor

@PierreQuinton PierreQuinton commented Oct 20, 2025

Some prototype of diagonal sparse Tensors.

TODO:

  • Add assert utility to test that the DSTs are the same whence densified and also the shape of contiguous_data/v_to_ps match up to joint move_dim.
  • (Probably later, for now we want to keep getting assertions error) Catch errors in dispatch to fall back to dense for maximal compatibility.

Problems and questions:

  • Slicing is done on each dim independently, if we had t=DST(..., v_to_p=[[0],[0]]) and slicing t[2:8, 2:8], we would want the result to be a DST(..., v_to_p=[[0], [0]]) but this cannot be done easily with slicing iteratively on each dimensions.
  • Should we use the operator from aten on inner Tensor rather than higher level operators from torch? It feels like aten do not accept any Tensor and we would want to be able to compose with any. However doing this might result in some extra overhead.

@PierreQuinton PierreQuinton added feat New feature or request package: autogram labels Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

❌ Patch coverage is 94.79167% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/torchjd/autogram/diagonal_sparse_tensor.py 94.68% 5 Missing ⚠️
Files with missing lines Coverage Δ
src/torchjd/autogram/_engine.py 100.00% <100.00%> (ø)
src/torchjd/autogram/diagonal_sparse_tensor.py 94.68% <94.68%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ValerianRey ValerianRey changed the base branch from main to dev-new-engine October 22, 2025 18:19
@ValerianRey ValerianRey force-pushed the block-diagonal-tensor branch from 87b4da0 to efa8019 Compare October 23, 2025 01:30
@PierreQuinton
Copy link
Contributor Author

I think square is failing because it is implemented as t * t and we didn't implement mul yet.

ValerianRey and others added 22 commits October 30, 2025 08:04
…irtual dimension that uses a physical dimension multiple times.
* The result is the same as before
* Before that we only iterated on the pdims used by each virtual dim, and summed them if a pdim was present multiple times.
* Now the new stride is already the sum of the old strides when a pdim is present multiple times in a vdim. We iterate over all dimensions, because for dimensions not present in the vdim, the stride is simply 0.
* There's probably a more efficient implementation
* Now that we iterate over all_pdims instead of the pdims of the current virtual dimension, the result of torch.stack([p_indices_grid[d] for d in all_pdims], dim=-1) is always the same, and is simply equal to torch.stack(p_indices_grid, dim=-1). So we directly stack the p_indices_grid when creating it, and use the already stacked p_indices_grid in the for-loop.
* In the long term I'll try to rely mostly or even only on them instead of v_to_ps, so it makes sense to pre-compute them.
* This makes lines shorter
…cannot have strides on cuda because addmm_cuda (required for tensordot) does not support Long tensors (but the cpu version does).
self.physical = physical
self.v_to_ps = v_to_ps
pshape = list(self.physical.shape)
self.strides = tensor([strides_v2(pdims, pshape) for pdims in self.v_to_ps])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we make strides_v2 return a Tensor directly so that if we change from v_to_ps to pure stride description, we can then already assume it is an array (we could have a function to build a stride from a v_to_ps and physical.shape)

# addmm_cuda not implemented for Long tensors => gotta have these tensors on cpu
v_indices_grid = tensordot(self.strides, p_indices_grid, dims=1)
res = zeros(self.shape, device=self.device, dtype=self.dtype)
res[tuple(v_indices_grid)] = self.physical
Copy link
Contributor Author

@PierreQuinton PierreQuinton Oct 31, 2025

Choose a reason for hiding this comment

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

I would be really surprised if the cast to tuple was necessary (but did not check)

Suggested change
res[tuple(v_indices_grid)] = self.physical
res[v_indices_grid] = self.physical

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat New feature or request package: sparse

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants