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

Fix TrackedReal nextfloat and prevfloat #111

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Conversation

avik-pal
Copy link
Collaborator

As discussed with @DhairyaLGandhi it would make sense to move these functions to Tracker.jl

@yebai
Copy link
Member

yebai commented Aug 25, 2020

It's a good idea to consider transferring Tracker.jl specific rules back to Tracker.jl, perhaps under a dedicated folder for Distributions.jl. More generally, we should consider transferring implementations related to Zygote.jl, ReverseDiff.jl and ForwardDiff.jl back to their main repo as well.

However, we don't currently have merge access for repos mentioned above. It would be helpful if some Turing members (e.g. @mohamed82008, @willtebbutt, @devmotion and @yebai) are allowed to maintain these repos such that bug fixes can be merged quickly.

@willtebbutt
Copy link
Member

willtebbutt commented Aug 25, 2020

I would not be pro- this happening. Now that JuliaDiff/ChainRulesCore.jl#182 is in, Tracker and ReverseDiff should join Zygote and move over to ChainRules.

The @adjoints currently implemented should be replaced with rrules.

Tracker- and ReverseDiff-specific rules should be removed.

@mohamed82008
Copy link
Member

Ya I agree with Will, ChainRules is the future. May not be worth moving things around now unless they are urgently needed by users who don't want to load DistributionsAD.

@yebai
Copy link
Member

yebai commented Aug 25, 2020

Maintaining so many AD libraries is probably not the most fabulous idea in my view. In particular, Tracker and ReverseDiff are the same apart from implementation details.

@yebai yebai merged commit 02923db into TuringLang:master Aug 27, 2020
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.

4 participants