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 for HMCs dot_assume #1758

Merged
merged 13 commits into from
Jan 25, 2022
Merged

Fix for HMCs dot_assume #1758

merged 13 commits into from
Jan 25, 2022

Conversation

torfjelde
Copy link
Member

Currently dot_assume for HMC is broken, which this PR fixes.

This case is covered by tests, but (AFAIK) only in DynamicPPL's integration tests, not in Turing's tests.

Should I copy-paste these or is there a way we can just run the integration tests from DPPL with this Turing version?

@devmotion
Copy link
Member

Should I copy-paste these or is there a way we can just run the integration tests from DPPL with this Turing version?

I think the test setup is simpler and more reliable if you just copy the tests (I guess we could use the test models but probably that's what's done already in DynamicPPL?).

@torfjelde
Copy link
Member Author

Eeeh weird. I can't reproduce the errors locally 😳

@torfjelde
Copy link
Member Author

Aaaaight so I finally figured out what's going wrong:

julia> using Tracker, Distributions

julia> m = first(param(zeros(1)))
0.0 (tracked)

julia> s = first(param(ones(1)))
1.0 (tracked)

julia> typeof(Normal.(m, s))
Tracker.Tracked{Normal{Float64}}

And DistributionsAD.jl doesn't address this issue:

julia> using DistributionsAD

julia> typeof(Normal.(m, s))
Tracker.Tracked{Normal{Float64}}

It comes down to Tracker not handling broadcast properly. I'm just going to mark this test as broken for now (don't think anyone uses Tracker with Turing anyways) and raise an issue over at Tracker.jl.

@devmotion
Copy link
Member

Maybe related to or even the same as FluxML/Tracker.jl#65?

@torfjelde
Copy link
Member Author

Possibly! I've made a new issue as it's not completely obvious to me whether it's the same or not atm: FluxML/Tracker.jl#119

@torfjelde
Copy link
Member Author

Now running into #1756

@torfjelde
Copy link
Member Author

Renamed Core to TuringCore. Should we maybe bump the minor version? Seems like this could break people's code (e.g. I know lots of my snippets will break).

@torfjelde
Copy link
Member Author

This should be good to go once #1762 has gone through 👍

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #1758 (1d8eb1e) into master (9087412) will increase coverage by 0.63%.
The diff coverage is 100.00%.

❗ Current head 1d8eb1e differs from pull request most recent head b1a8195. Consider uploading reports for the commit b1a8195 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1758      +/-   ##
==========================================
+ Coverage   81.23%   81.87%   +0.63%     
==========================================
  Files          24       24              
  Lines        1471     1473       +2     
==========================================
+ Hits         1195     1206      +11     
+ Misses        276      267       -9     
Impacted Files Coverage Δ
src/inference/hmc.jl 79.39% <100.00%> (+3.03%) ⬆️
src/inference/AdvancedSMC.jl 97.47% <0.00%> (+0.04%) ⬆️
src/essential/ad.jl 86.07% <0.00%> (+5.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9087412...b1a8195. Read the comment docs.

@coveralls
Copy link

coveralls commented Jan 14, 2022

Pull Request Test Coverage Report for Build 1698293351

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 81.874%

Totals Coverage Status
Change from base Build 1696660269: 0.6%
Covered Lines: 1206
Relevant Lines: 1473

💛 - Coveralls

@yebai yebai merged commit b9f180a into master Jan 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the tor/hmc-fix branch January 25, 2022 10:43
yebai added a commit that referenced this pull request Jan 25, 2022
yebai added a commit that referenced this pull request Jan 26, 2022
* fixed dot_assume for hmc

* copy-pasted tests from dynamicppl integration tests

* inspecting what in the world is going on with tests

* trying again

* skip failing test for TrackerAD

* bump patch version

* fixed typo in tests

* Rename `Turing.Core` to `Turing.Essential`

* Deprecate Turing.Core

Co-authored-by: Tor Erlend Fjelde <[email protected]>

* fixed a numerical test

* version bump

Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
yebai added a commit that referenced this pull request Jan 26, 2022
* fixed dot_assume for hmc

* copy-pasted tests from dynamicppl integration tests

* inspecting what in the world is going on with tests

* trying again

* skip failing test for TrackerAD

* bump patch version

* fixed typo in tests

* Rename `Turing.Core` to `Turing.Essential`

* Deprecate Turing.Core

Co-authored-by: Tor Erlend Fjelde <[email protected]>

* fixed a numerical test

* version bump

Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
yebai added a commit that referenced this pull request Jan 26, 2022
yebai added a commit that referenced this pull request Jan 26, 2022
* fixed dot_assume for hmc

* copy-pasted tests from dynamicppl integration tests

* inspecting what in the world is going on with tests

* trying again

* skip failing test for TrackerAD

* bump patch version

* fixed typo in tests

* Rename `Turing.Core` to `Turing.Essential`

* Deprecate Turing.Core

Co-authored-by: Tor Erlend Fjelde <[email protected]>

* fixed a numerical test

* version bump

Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
yebai added a commit that referenced this pull request Jan 31, 2022
* New Turing-libtask integration  (#1757)

* Update Project.toml

* Update Project.toml

* Update Project.toml

* trace down into functions calling produce

* trace into functions in testcases

* update to the latest version

* run tests against new libtask

* temporarily disable 1.3 for testing

* Update AdvancedSMC.jl

* Update AdvancedSMC.jl

* Update AdvancedSMC.jl

* Update AdvancedSMC.jl

* Update AdvancedSMC.jl

* copy Trace on tape

* Implement simplified evaluator for TracedModel

* Remove some unnecessary trace functions.

* Minor bugfix in TracedModel evaluator.

* Update .github/workflows/TuringCI.yml

* Minor bugfix in TracedModel evaluator.

* Update container.jl

* Update Project.toml

* Commented out tests related to control flow.  TuringLang/Libtask.jl/issues/96

* Commented out tests related to control flow.
TuringLang/Libtask.jl/issues/96

* Update Project.toml

* Update src/essential/container.jl

* Update AdvancedSMC.jl

Co-authored-by: KDr2 <[email protected]>

* CompatHelper: add new compat entry for Libtask at version 0.6 for package test, (keep existing compat) (#1765)

Co-authored-by: CompatHelper Julia <[email protected]>

* Fix for HMCs `dot_assume` (#1758)

* fixed dot_assume for hmc

* copy-pasted tests from dynamicppl integration tests

* inspecting what in the world is going on with tests

* trying again

* skip failing test for TrackerAD

* bump patch version

* fixed typo in tests

* Rename `Turing.Core` to `Turing.Essential`

* Deprecate Turing.Core

Co-authored-by: Tor Erlend Fjelde <[email protected]>

* fixed a numerical test

* version bump

Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>

* Minor fixes.

* Minor fixes.

* Minor fix.

* Update Julia version in CI

* Merge branch 'libtask-integration' of github.com:TuringLang/Turing.jl into libtask-integration

* Update Inference.jl

* Minor fixes.

* Add back `imm` test.

* Minor tweaks to make single
distribution tests more robust.

* Update Project.toml

Co-authored-by: David Widmann <[email protected]>

* Apply suggestions from code review

Co-authored-by: David Widmann <[email protected]>

* Update Project.toml

* Switch to StableRNGs for broken tests.

* Apply suggestions from code review

Co-authored-by: David Widmann <[email protected]>

* Minor tweaks.

* Use StableRNG for GMM test.

* Update Project.toml

Co-authored-by: KDr2 <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
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.

5 participants