Skip to content

Conversation

@kunzaatko
Copy link

@kunzaatko kunzaatko commented Feb 22, 2025

A method for specifying the indexes to fill with values directly as
CartesianIndices.

For example:

julia> sparse([CartesianIndex(1,1), CartesianIndex(2,2), CartesianIndex(3,3)], [1.0, 2.0, 3.0], 3, 3)

is now possible.

  • documentation?
  • Additional tests?

A method for specifying the indexes to fill with values directly as
CartesianIndices.
@codecov
Copy link

codecov bot commented Feb 22, 2025

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.19%. Comparing base (ce852af) to head (42e29b6).
⚠️ Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
src/sparsematrix.jl 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
+ Coverage   84.08%   84.19%   +0.11%     
==========================================
  Files          12       12              
  Lines        9192     9309     +117     
==========================================
+ Hits         7729     7838     +109     
- Misses       1463     1471       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kunzaatko
Copy link
Author

Another method that should be considered are ones that are created by using a range on instances of CartesianIndex

julia> CartesianIndex(3,4):CartesianIndex(4,9)
CartesianIndices((3:4, 4:9))

julia> CartesianIndex(3,4):CartesianIndex(4,9) |> typeof |> supertypes
(CartesianIndices{2, Tuple{UnitRange{Int64}, UnitRange{Int64}}}, AbstractMatrix{CartesianIndex{2}}, Any)

@kunzaatko kunzaatko marked this pull request as ready for review February 22, 2025 09:16
@jishnub
Copy link
Contributor

jishnub commented Feb 22, 2025

Note that the CartesianIndices block above is dense (although it might be useful as the indices of a block matrix). A sparse range along a 1D axis may be created as a StepRangeLen of CartesianIndexes.

@kunzaatko
Copy link
Author

kunzaatko commented Feb 23, 2025

Perhaps what you are describing is a more realistic use of the dense block. That was what I had in mind although the sparse function does not give an ideal interface for this use, am I correct? Something like this does not have much overhead thought (and maybe any overhead is even compiled out?)

julia> sparse(CartesianIndex(12:14):CartesianIndex(15:17), ones(3*3), 20, 20) + sparse(CartesianIndex(2:4):CartesianIndex(5:7), ones(3*3), 20, 20)

Which would create a sparse matrix with dense blocks inside. The structure of the type does not give the best memory usage for this case...

NOTE: `CartesianIndex{2}` is no longer an abstract type so the function
no longer needs to be parametric.
@ViralBShah
Copy link
Member

Should we merge this?

@kunzaatko
Copy link
Author

kunzaatko commented Jan 6, 2026

With the exception of documentation, I think there is nothing more to add to the constructor that allows for passing 2D CartesianIndex in the list of indices. As for the suggestion of @jishnub, that it should be possible to create a sparse matrix from dense blocks with CartesianIndies, we can transfer that into a new feature issue and discuss that there. I will type up some documentation and update the documentation pages. Do you have any suggestion of what else should be tested for this constructor so that I can add it to the test suite? @ViralBShah

@kunzaatko
Copy link
Author

@ViralBShah I think this is ready for merge if the CI passes. I added the documentation in the latest commit. You may check whether it is to your liking.

@kunzaatko
Copy link
Author

There is a problem with ambiguities. I guess that I need to explicitly typeup all the methods

Also fixed the methods to include all of the shortcuts (defaults) that
the multiple vector method supports.
@kunzaatko
Copy link
Author

I am not sure what the best way to combat these ambiguities is. As I am not familiar with the code base, I am not sure, whether it would be OK, to assume that I in sparse(I,...) is an AbstractVector. That would solve the issue.

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.

3 participants