Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing / incorrect impl of Base.parent for LogDensityProblemsAD.jl interface #197

Closed
torfjelde opened this issue Jul 18, 2024 · 10 comments
Closed
Labels
enhancement New feature or request high priority

Comments

@torfjelde
Copy link

LogDensityProblemsAD.jl implements Base.parent for ADGradientWrapper (https://github.com/tpapp/LogDensityProblemsAD.jl/blob/449e5661bc2667f7bef061e148a6ea5526cbb427/src/LogDensityProblemsAD.jl#L34) by just accessing the field, which is assumed to return the original logdensity passed to ADgradient.

IIRC that field is not the original logdensity but a Tapir.CoDual? Hence it might be worth overloading Base.parent to return the "correct" thing, or alternatively change what the field represents.

Lack of this will cause issues in applications where one wants to re-construct the gradient wrapper with a new logdensity programmatically, e.g. TuringLang/Turing.jl#2231 (and more specifically, TuringLang/Turing.jl#2231 (comment)).

@torfjelde
Copy link
Author

Was about to make a PR myself, but figured I should first check if @willtebbutt is happy with this:)

@yebai
Copy link
Contributor

yebai commented Jul 18, 2024

The issue has also been noted in TuringLang/Turing.jl#2289 (comment). @torfjelde, feel free to create a PR.

@yebai yebai added the bug Something isn't working label Jul 18, 2024
@willtebbutt
Copy link
Member

Ah, interesting. Is the field technically part of the public interface for ADGradientWrapper? This wasn't clear to me from the docs of the package, so I assumed that I had the freedom to choose my own field names / do what I like with the fields.

In my view, adding a method of Base.parent makes most sense. You should use Tapir.primal to extract the bit of the field that you're interested in.

@torfjelde
Copy link
Author

Ah, interesting. Is the ℓ field technically part of the public interface for ADGradientWrapper? This wasn't clear to me from the docs of the package, so I assumed that I had the freedom to choose my own field names / do what I like with the fields.

Tbh, I don't know 😅 I read the src code as Base.parent being part of the API and the default to just being a, well, default 🤷 So in agreement with what you're saying: overload Base.parent seems to be the way to go

@willtebbutt
Copy link
Member

willtebbutt commented Jul 23, 2024

Version 0.2.25 of Tapir.jl should resolve this. Could someone please check?

@yebai
Copy link
Contributor

yebai commented Jul 23, 2024

I triggered a rerun of TuringLang/Turing.jl#2289.

@yebai
Copy link
Contributor

yebai commented Jul 23, 2024

It seems there are additional places that need fix: https://github.com/TuringLang/Turing.jl/actions/runs/9988691426/job/27802772815?pr=2289#step:8:775

@willtebbutt
Copy link
Member

I've commented directly on the PR associated to that run -- it looks more like a Turing.jl issue than a Tapir.jl issue to me, but I might be misinterpreting.

@torfjelde
Copy link
Author

You are indeed correct @willtebbutt 👍

This was referenced Jul 29, 2024
@willtebbutt willtebbutt added enhancement New feature or request high priority and removed bug Something isn't working labels Jul 30, 2024
@yebai yebai closed this as completed Aug 5, 2024
@yebai
Copy link
Contributor

yebai commented Aug 5, 2024

Closing this since the remaining issues with Turing are caused by separate reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high priority
Projects
None yet
Development

No branches or pull requests

3 participants