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

CompatHelper: bump compat for Bijectors to 0.13, (keep existing compat) #2018

Merged

Conversation

github-actions[bot]
Copy link
Contributor

This pull request changes the compat entry for the Bijectors package from 0.12 to 0.12, 0.13.
This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry.
It is your responsibility to make sure that your package tests pass before you merge this pull request.

@cpfiffer cpfiffer force-pushed the compathelper/new_version/2023-06-20-00-28-18-752-01921173878 branch from ccd5b3e to 04ce8a6 Compare June 20, 2023 00:28
@github-actions
Copy link
Contributor Author

Pull Request Test Coverage Report for Build 5317209593

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 5313671228: 0.0%
Covered Lines: 0
Relevant Lines: 1427

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (1b67694) 0.00% compared to head (e42eec9) 0.00%.

❗ Current head e42eec9 differs from pull request most recent head bc815c0. Consider uploading reports for the commit bc815c0 to get more accurate results

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2018   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          22      22           
  Lines        1473    1473           
======================================
  Misses       1473    1473           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yebai
Copy link
Member

yebai commented Jun 24, 2023

@harisorgn It seems some fixes are probably required here as well to support new LKJ priors.

@harisorgn
Copy link
Member

@harisorgn It seems some fixes are probably required here as well to support new LKJ priors.

Sorry, promised to have a look but couldn't make it before starting my holidays. If someone else wants to take this over, please feel free. Otherwise I'll try to fix tests by the end of next week : )

@yebai yebai mentioned this pull request Jul 14, 2023
@yebai
Copy link
Member

yebai commented Jul 14, 2023

A gentle reminder @harisorgn

* Fix testset for external samplers

* Update abstractmcmc.jl
@yebai
Copy link
Member

yebai commented Jul 16, 2023

#2040 fixed some formatting issues for CI results. No functionality change was added.

@torfjelde
Copy link
Member

TuringLang/DynamicPPL.jl#499 should address the failing tests

@harisorgn
Copy link
Member

harisorgn commented Jul 17, 2023

TuringLang/DynamicPPL.jl#499 should address the failing tests

thanks @torfjelde . Found something that probably still needs to be addressed. It's the change to fill! in triu_mask in Bijectors.jl . Basically fill!(x::Union{UpperTriangular, LowerTriangular}, some_value) complains that it can not access matrix indices that do not belong to the relevant triangular. Not sure if this is expected behaviour or a LinearAlgebra bug, seems like the latter to me, will open an issue there.

We can revert to the old way without fill! (you recently changed it IIRC).

@torfjelde
Copy link
Member

We can revert to the old way without fill! (you recently changed it IIRC).

Maybe easier to just call parent on the input, no?

@harisorgn
Copy link
Member

harisorgn commented Jul 17, 2023

Maybe easier to just call parent on the input, no?

Yes, see, that's why I mentioned the problem before trying to fix it. Your way is easier (edit: and better overall), thanks 😅

@yebai
Copy link
Member

yebai commented Jul 20, 2023

TuringLang/DynamicPPL.jl#503 and #2049 should fix all remaining tests.

EDIT: there is still some issue with Bijectors.

* Update OptimInterface.jl

* Only run optimisation tests in numerical stage.

* fix function lookup after moving functions

---------

Co-authored-by: Xianda Sun <[email protected]>
@yebai
Copy link
Member

yebai commented Jul 21, 2023

After #2049, we have only two AD-related errors remaining. They are likely related to recent Bijectors changes, see e.g., #2049 (comment)

@torfjelde
Copy link
Member

I've made a fix for ReverseDiff in TuringLang/Bijectors.jl#280, but this won't work for Tracker since it does not support ChainRulesCore.

@torfjelde
Copy link
Member

I believe after TuringLang/Bijectors.jl#280 and TuringLang/DynamicPPL.jl#513, CI here should be fairly likely to pass

@torfjelde
Copy link
Member

Btw, are we bothered about Tracker.jl tests failing, given that we're now warning the user that Tracker isn't officially supported anymore?

@devmotion
Copy link
Member

I guess in principle it's fine to have worse support for Tracker - but then I think we should change the default reverse-mode backend to e.g. ReverseDiff. However, in this case here it seems the fix in Bijectors can be extended to Tracker without much effort by using the same pullbacks?

@torfjelde
Copy link
Member

Once Bijectors has been released, I'll bump the compat version of the tests and give this another go:)

Project.toml Outdated Show resolved Hide resolved
Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Thanks, @torfjelde @harisorgn -- please feel free to merge if CI passes.

@torfjelde
Copy link
Member

A bit confused as to why we're getting complaints still; we shouldn't even be hitting cholesky with TrackedArray anymore.

@yebai
Copy link
Member

yebai commented Aug 14, 2023

@torfjelde It looks like a Julia 1.7 issue. The following tests run successfully on Julia 1.9, but fails on Julia 1.7:

julia> using Turing, LinearAlgebra, ReverseDiff

julia> @model function wishart()
           theta ~ Wishart(4, Matrix{Float64}(I, 4, 4))
       end
wishart (generic function with 2 methods)

julia> Turing.setadbackend(:reversediff)
:reversediff

julia> sample(wishart(), HMC(0.01, 1), 1000)

@torfjelde
Copy link
Member

Hmm, need to check if there's a discrepancy between 1.6 and 1.7 then, since the tests were passing on 1.6 with Bijectors

@yebai
Copy link
Member

yebai commented Aug 14, 2023

It might be caused by other parts of Turing, e.g. DynamicPPL. CI tests for Bijectors pass for all Julia releases betwee 1.6-1.9. See TuringLang/Bijectors.jl#284

Project.toml Outdated Show resolved Hide resolved
@yebai
Copy link
Member

yebai commented Aug 16, 2023

@torfjelde All tests for ReverseDiff pass, but this revealed one failure case for Zygote. Can you take a look?

julia> using Turing, LinearAlgebra, Zygote

julia> @model function wishart()
           theta ~ Wishart(4, Matrix{Float64}(I, 4, 4))
       end
wishart (generic function with 2 methods)

julia> Turing.setadbackend(:zygote)
:zygote

julia> sample(wishart(), HMC(0.01, 1), 1000)

@coveralls
Copy link

coveralls commented Aug 16, 2023

Pull Request Test Coverage Report for Build 5882947478

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 5860942594: 0.0%
Covered Lines: 0
Relevant Lines: 1473

💛 - Coveralls

@yebai yebai merged commit d8beaf0 into master Aug 16, 2023
8 of 11 checks passed
@yebai yebai deleted the compathelper/new_version/2023-06-20-00-28-18-752-01921173878 branch August 16, 2023 21:43
@torfjelde
Copy link
Member

Thanks for finishing this off Hong!

@harisorgn
Copy link
Member

Thanks for finishing this off Hong!

Seconded 🙏

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.

7 participants