Skip to content

Move duplicated AD helpers into main package#48

Open
kshyatt wants to merge 1 commit into
mainfrom
ksh/common_factors
Open

Move duplicated AD helpers into main package#48
kshyatt wants to merge 1 commit into
mainfrom
ksh/common_factors

Conversation

@kshyatt

@kshyatt kshyatt commented Jun 16, 2026

Copy link
Copy Markdown
Member

Surfaced elsewhere but it would be good to start consolidating all these

@kshyatt kshyatt requested review from Jutho and lkdvos June 16, 2026 19:52
Comment thread src/ad.jl
function project_add!(C, A, α)
TC = Base.promote_op(+, scalartype(A), scalartype(α))
return if !(TC <: Real) && scalartype(C) <: Real
add!(C, real(add!(zerovector(C, TC), A, α)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The problem with this definition is of course that it is much more limited than the types that VI supports, because of the real call. Effectively, this will only support AbstractArray{<:Number}, not nested arrays like add! does.

I don't think there is a quick solution for that. One way might have to have a a project_add! implementation that is similar to add!, but that forces to also have project_add!! with implementations for tuples etc, as that is how add! is implemented.

An alternative, which might be useful in other places as well, is that VectorInterface adds a new method to its interface, namely project_real, that supports nested types. I certainly have contemplated about this in other places as well where I thought this could be useful, but then always found another way around this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I suppose one option would be to require such types to implement their own project_add!, which they can at least do more easily if it's actually a "first class citizen" package method?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For other types, yes, but I mean, the current implementation already fails for something like A=[(1., 2.), (3., 4.)] which is currently supported by VectorInterface.add!.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Then indeed maybe project_real is a good option, if nothing else so that examples like that don't blow up beneath people unexpectedly

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interested to also hear @lkdvos' opinion on this.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/ad.jl 83.33% 1 Missing ⚠️
Files with missing lines Coverage Δ
ext/VectorInterfaceMooncakeExt.jl 96.05% <ø> (+0.93%) ⬆️
src/VectorInterface.jl 100.00% <ø> (ø)
src/ad.jl 83.33% <83.33%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants