Skip to content

Conversation

@kshyatt
Copy link
Member

@kshyatt kshyatt commented Oct 15, 2025

No description provided.

@kshyatt kshyatt requested a review from lkdvos October 15, 2025 22:21
@github-actions
Copy link

github-actions bot commented Oct 15, 2025

Your PR no longer requires formatting changes. Thank you for your contribution!

@kshyatt
Copy link
Member Author

kshyatt commented Oct 15, 2025

This will need #80 to merge first

@kshyatt kshyatt enabled auto-merge (squash) October 16, 2025 09:16
Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this and then deal with the possible orth changes, or the other way around. I'm not sure what is easier?

@kshyatt
Copy link
Member Author

kshyatt commented Oct 16, 2025

I'd say merge this once I get the AMDGPU tests passing, then I can do another PR post your changes?

@kshyatt
Copy link
Member Author

kshyatt commented Oct 16, 2025

@lkdvos I had to modify the truncate a little to play nicely with 1.10 and AMDGPU -- the result (and the number of allocs) should be similar, but GPUCompiler really didn't like the logical indexing and this was least ugly way I found to set this up...

@codecov
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
...ixAlgebraKitAMDGPUExt/MatrixAlgebraKitAMDGPUExt.jl 76.92% <100.00%> (+12.47%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Looks good overall to me. I wasn't sure if we should first merge this or first the ortho rework, but it looks like that might still require some additional time/reviews so LGTM first

U, S = US
extended_S = vcat(diagview(S), zeros(eltype(S), max(0, size(S, 1) - size(S, 2))))
ind = MatrixAlgebraKit.findtruncated(extended_S, strategy)
trunc_cols = collect(1:size(U, 2))[ind]
Copy link
Member

@lkdvos lkdvos Oct 17, 2025

Choose a reason for hiding this comment

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

Suggested change
trunc_cols = collect(1:size(U, 2))[ind]
trunc_cols = ind isa AbstractVector{Integer} ? ind : axes(U, 2)[ind]

Does this happen to work as well?

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Left one last comment more out of curiosity than actual importance, otherwise good to go for me!

@kshyatt kshyatt merged commit 4fbc3bf into main Oct 17, 2025
10 checks passed
@kshyatt kshyatt deleted the ksh/orthnull branch October 17, 2025 13:36
@lkdvos lkdvos mentioned this pull request Nov 14, 2025
lkdvos referenced this pull request Nov 14, 2025
* Bump v0.6

* rename `gaugefix` -> `fixgauge`

* reduce unnecessary warnings

* fix `copy_input` signatures in Mooncake tests

* Add changelog to docs
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