Skip to content

Conversation

@jishnub
Copy link
Contributor

@jishnub jishnub commented Dec 4, 2024

Currently, only istriu(S) and istril(S) are specialized for sparse matrices, and this PR generalizes these to accept the band index k. This improves performance:

julia> S = sparse(1:4000, 1:4000, 1:4000);

julia> @btime istriu($S, -1);
  20.941 ms (0 allocations: 0 bytes) # main
  5.660 μs (0 allocations: 0 bytes) # this PR

@codecov
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.07%. Comparing base (780c4de) to head (598da1f).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #590   +/-   ##
=======================================
  Coverage   84.07%   84.07%           
=======================================
  Files          12       12           
  Lines        9188     9188           
=======================================
  Hits         7725     7725           
  Misses       1463     1463           

☔ 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.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

Nice!

@jishnub jishnub merged commit 4fd3aad into main Dec 5, 2024
10 checks passed
@jishnub jishnub deleted the jishnub/istriul branch December 5, 2024 08:09
@jonas-schulze
Copy link

It seems that Julia 1.12 is the first version to ship this. It would be nice to have this in the current LTS. What do you think?

@ViralBShah
Copy link
Member

ViralBShah commented Aug 12, 2025

Since LTS and older releases can strictly only have bugfixes and no new features or capabilities, I do not think backporting to LTS will be feasible.

I believe this should already be on 1.12 - but it would be good to confirm with a Julia RC if that is indeed the case. If not, we should mark it for backport to 1.12.

@jonas-schulze
Copy link

Since LTS and older releases can strictly only have bugfixes and [no] new features or capabilities, I do not think backporting to LTS will be feasible.

I would argue this causes a performance bug of isdiag(::SparseMatrixCSC).

I believe this should already be on 1.12 - but it would be good to confirm with a Julia RC if that is indeed the case. If not, we should mark it for backport to 1.12.

It is present in 1.12.0-rc1.

@ViralBShah
Copy link
Member

That's getting a little cute to get it in. I think it is small and simple enough that we can try.

@jishnub
Copy link
Contributor Author

jishnub commented Aug 12, 2025

I don't think performance improvements are backported unless it's to fix a regression.

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.

5 participants