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

Bump DynamicPPL to v0.25 #2197

Merged
merged 17 commits into from Apr 23, 2024
Merged

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented Apr 19, 2024

Replaces Setfield.jl with Accessors.jl.

EDIT: Won't work until DPPL 0.25 is released.

@torfjelde torfjelde requested a review from sunxd3 April 19, 2024 19:14
@torfjelde
Copy link
Member Author

@sunxd3 it's good practice to let the author of a branch know if you contribute to it:) I was very confused as to what was going wrong here until I realized you had merged changes 😅

@sunxd3
Copy link
Collaborator

sunxd3 commented Apr 20, 2024

Fair enough! Sorry about the confusion

@torfjelde
Copy link
Member Author

@sunxd3 any clue what's going on here?

@sunxd3
Copy link
Collaborator

sunxd3 commented Apr 20, 2024

No idea -- interesting it's only failing windows but not ubuntu

@sunxd3
Copy link
Collaborator

sunxd3 commented Apr 20, 2024

@torfjelde
Copy link
Member Author

No idea -- interesting it's only failing windows but not ubuntu

I think it's Julia 1.7 that's the issue, not windows vs linux

@torfjelde
Copy link
Member Author

Aye, it's the difference between 1.7 and 1.10.

In particular, on 1.7 we hit the following impl of copyto!

function copy!(dst::AbstractVector, src::AbstractVector)
    if length(dst) != length(src)
        resize!(dst, length(src))
    end
    for i in eachindex(dst, src)
        @inbounds dst[i] = src[i]
    end
    dst
end

while on 1.10 we first hit

function copy!(dst::AbstractVector, src::AbstractVector)
    firstindex(dst) == firstindex(src) || throw(ArgumentError(
        "vectors must have the same offset for copy! (consider using `copyto!`)"))
    if length(dst) != length(src)
        resize!(dst, length(src))
    end
    copyto!(dst, src)
end

which then eventually hits

function _copyto_impl!(dest::Array, doffs::Integer, src::Array, soffs::Integer, n::Integer)
    n == 0 && return dest
    n > 0 || _throw_argerror("Number of elements to copy must be nonnegative.")
    @boundscheck checkbounds(dest, doffs:doffs+n-1)
    @boundscheck checkbounds(src, soffs:soffs+n-1)
    unsafe_copyto!(dest, doffs, src, soffs, n)
    return dest
end

i.e. we hit unsafe_copyto!, which I assume has no such check for undef and hence works without any issues. In contrast, the 1.7 version requires getindex impl.

Tbh, I'm a bit uncertain what's "correct" here (and it's definitively a BangBang.jl issue rather than something we're doing wrong).

it's hitting NoBang

Preferably not, but not surprised. BangBang.jl have fairly rudimentary checks to decide whether to mutate or not.

@torfjelde
Copy link
Member Author

Fix: JuliaFolds2/BangBang.jl#22

@torfjelde
Copy link
Member Author

Note that this is a breaking change due to how the varnames are now shown, e.g. x[1,2] before is now x[1, 2].

@torfjelde
Copy link
Member Author

torfjelde commented Apr 22, 2024

Any thoughts on what we should do here @devmotion @yebai @sunxd3 ? This issue is holding up a lot of PRs now 😕

It might be worth disabling the failing test on Julia 1.7 given the depth of the issue

@sunxd3
Copy link
Collaborator

sunxd3 commented Apr 22, 2024

disabling the failing test on Julia 1.7

I don't mind.
Looks like it's only failing on array of vectors? maybe we can just warn that this use case won't work on 1.7?

Do we want to stop supporting 1.7 (given the LTS is 1.9)?

@torfjelde
Copy link
Member Author

LTS is still 1.6, no?

@torfjelde
Copy link
Member Author

I removed the failing test for Julia <1.8 for now since it's not "our fault". Once JuliaFolds2/BangBang.jl#22 goes through, we'll bring it back

@torfjelde
Copy link
Member Author

Tests should be passing now

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8799654893

Details

  • 0 of 13 (0.0%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 0.0%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/mcmc/abstractmcmc.jl 0 1 0.0%
src/optimisation/Optimisation.jl 0 1 0.0%
src/experimental/gibbs.jl 0 4 0.0%
ext/TuringOptimExt.jl 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
ext/TuringOptimExt.jl 1 0.0%
Totals Coverage Status
Change from base Build 8771748446: 0.0%
Covered Lines: 0
Relevant Lines: 1511

💛 - Coveralls

@torfjelde torfjelde merged commit 9be6b79 into master Apr 23, 2024
11 checks passed
@torfjelde torfjelde deleted the torfjelde/accessors-instead-of-setfield branch April 23, 2024 15:56
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.

None yet

3 participants