Skip to content

Conversation

@lkdvos
Copy link
Member

@lkdvos lkdvos commented Feb 4, 2026

No description provided.

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

❌ Patch coverage is 83.43949% with 26 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...gebraKitMooncakeExt/MatrixAlgebraKitMooncakeExt.jl 83.43% 26 Missing ⚠️
Files with missing lines Coverage Δ
...gebraKitMooncakeExt/MatrixAlgebraKitMooncakeExt.jl 60.75% <83.43%> (-36.03%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lkdvos lkdvos marked this pull request as ready for review February 4, 2026 19:16
@lkdvos lkdvos requested review from Jutho and kshyatt February 4, 2026 19:16
Comment on lines 241 to 247
# compute pullbacks
$f_pullback!(dA, Ac, DVc, dDVtrunc, ind)
zero!.(dDVtrunc) # since this is allocated in this function this is probably not required

# restore state
copy!(A, Ac)
copy!.(DV, DVc)
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with the other adjoints, the copy!(A, Ac) would happen before calling the pullback, and then you can just pass A in the f_pullback! call. I am not opposed to the current order/approach but do value consistency as it helps in reading the code at a later stage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I would like to do a follow up PR where I refactor a bunch of this into the same order and format, but didn't want to pollute this PR too much with simple formatting changes.

Comment on lines 358 to 364
# compute pullbacks
$f_pullback!(dA, Ac, DVc, dDVtrunc, ind)
zero!.(dDVtrunc) # since this is allocated in this function this is probably not required

# restore state
copy!(A, Ac)
copy!.(DV, DVc)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about order of operations.

@lkdvos
Copy link
Member Author

lkdvos commented Feb 5, 2026

I think I addressed most of the remarks here, except for the order of operations. Would you be okay with me leaving that for a follow-up PR? I would like to refactor some of this code a bit more, and I don't want to pollute this PR with a bunch of formatting changes if possible.

@lkdvos lkdvos merged commit a349aef into main Feb 5, 2026
10 checks passed
@lkdvos lkdvos deleted the ld-mooncake branch February 5, 2026 16:13
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