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 173 #341

Merged
merged 33 commits into from
Nov 16, 2017
Merged

Fix 173 #341

merged 33 commits into from
Nov 16, 2017

Conversation

yebai
Copy link
Member

@yebai yebai commented Aug 28, 2017

No description provided.

@yebai yebai mentioned this pull request Aug 28, 2017
@xukai92
Copy link
Member

xukai92 commented Oct 3, 2017

sample.jl (i.e. Gibbs with HMC + PG on gdemo) works on Julia 0.6 with 2 changes:

  1. Make model wrapper call and sample function call separate (ffb9407);
  2. Update to ForwardDiff.jl's new signature (2a9466d);

1. is just a work around for the test. Need to fix it.

@xukai92
Copy link
Member

xukai92 commented Oct 3, 2017

I figure out how to resolve the issue by using Base.invokelatest() - but we may need to make change to the signature of model because Base.invokelatest() doesn't support keyword arguments. (Note: out model signature is like model(vi=vi, sampler=spl) which is keyword argument based.)

@xukai92
Copy link
Member

xukai92 commented Oct 3, 2017

  • Fix 1. by changing the model signature and using invokelatest().
  • Fix warning messages as many as possible

@xukai92
Copy link
Member

xukai92 commented Oct 4, 2017

Most of the test are passed now. But Julia 0.6 (or other changes?) may lead to some minor changes to some cases. E.g.

p[i,j] ~ dist

generates a VarName called p[1, 2] (with space) now, which was p[1,2] (without space). This means we need to changes many of the tests/benchmarks for results fetching.

Any idea why?

@yebai
Copy link
Member Author

yebai commented Oct 4, 2017

This might be because we introduced a mechanism for translating ~ operator into a macro in Julia 0.6.

@xukai92
Copy link
Member

xukai92 commented Oct 4, 2017

@yebai I see.

All the tests passed on my local Linux machine now. I changed some deprecated functions but now all. I will do that in the next few days.

@yebai
Copy link
Member Author

yebai commented Oct 5, 2017

I pumped Julia version to 0.6 on Travis and Appveyor. The tests run on my Mac as well, but seems quite slow at the moment.

Ps. I got an error on Mac

[runtests.jl] "ad.jl/ad1.jl" is running
[Turing]:  Assume - `s` is a parameter
 @~(::ANY, ::ANY) at compiler.jl:76
[Turing]:  Assume - `m` is a parameter
 @~(::ANY, ::ANY) at compiler.jl:76
ERROR: LoadError: LoadError: MethodError: no method matching ##ad_test_model#1140()
Closest candidates are:
  ##ad_test_model#1140(::Turing.VarInfo, ::Union{Turing.Sampler, Void}) at /Users/yebai/.julia/v0.6/Turing/test/ad.jl/ad1.jl:10
Stacktrace:
 [1] include_from_node1(::String) at ./loading.jl:569
 [2] include(::String) at ./sysimg.jl:14
 [3] macro expansion at /Users/yebai/.julia/v0.6/Turing/test/runtests.jl:86 [inlined]
 [4] anonymous at ./<missing>:?
 [5] include_from_node1(::String) at ./loading.jl:569
 [6] include(::String) at ./sysimg.jl:14
 [7] process_options(::Base.JLOptions) at ./client.jl:305
 [8] _start() at ./client.jl:371
while loading /Users/yebai/.julia/v0.6/Turing/test/ad.jl/ad1.jl, in expression starting on line 20
while loading /Users/yebai/.julia/v0.6/Turing/test/runtests.jl, in expression starting on line 74

@xukai92
Copy link
Member

xukai92 commented Oct 5, 2017

Yes this is a bug in the test. My last commit fixes the test.

This is becauseI didn't actually implement the callback functions (I tried that naively but the compiler doesn't work as I expect) but always use modelf(vi::VarInfo, sampler::Sampler). In empty argument cases before I changed those codes to modelf(VarInfo(), nothing). It's wired my Linux doesn't give the error.

  • implement callback functions

@xukai92
Copy link
Member

xukai92 commented Oct 9, 2017

Known unfixed warnings:

  • WARNING: consume is now deprecated. Use Channels for inter-task communication.

@xukai92
Copy link
Member

xukai92 commented Oct 9, 2017

I also noticed Turing.jl becomes very slow after migrated to 0.6. I was guessing that's because those deprecations inside model function (which will be called mutiple times during model evatluation), e.g. exp vs exp.

julia> a = randn(100)

julia> @time for _ = 1:1000 exp(a) end
  0.496221 seconds (83.00 k allocations: 12.161 MiB, 1.33% gc time)

julia> @time for _ = 1:1000 exp.(a) end
  0.088814 seconds (49.79 k allocations: 3.175 MiB)

I've fixed most of them - seems become a bit faster.

@xukai92
Copy link
Member

xukai92 commented Oct 10, 2017

@yebai I finally resolve the callback issue.

In terms of the deprecation, I guess the only one unsolved is the Channel one. I don't completely understand what's going on with it. Do you think we can simply replace all Task with Channel?

@yebai
Copy link
Member Author

yebai commented Oct 10, 2017

In terms of the deprecation, I guess the only one unsolved is the Channel one. I don't completely understand what's going on with it. Do you think we can simply replace all Task with Channel?

I'm not sure about this yet; will take a look at the new Task interface.

@yebai
Copy link
Member Author

yebai commented Oct 11, 2017

Yes, we can use Channel in Julia 0.6, but we don't need to drop Task. Basically, the consume/produce API is replaced by take!/put!.

f(_c::Channel{Any}) = begin
      for i =1:1000
      println(i);put!(_c,i);
      end
end

a = Channel(f)

take!(a)

The part related to task copying might be a bit tricky. I will look into this further later.

Ref: JuliaLang/julia#19841

@yebai
Copy link
Member Author

yebai commented Oct 11, 2017

@xukai92 Seems the tests are failing. Could you take a look at Travis?

@yebai yebai mentioned this pull request Oct 18, 2017
@yebai
Copy link
Member Author

yebai commented Nov 9, 2017

@xukai92 I still haven't figured how to use channels correctly when tasks are copied. In the meantime, I implemented a temporary hack so that we can continue to use produce before I have a better solution.

# Conflicts:
#	src/samplers/pmmh.jl
@xukai92
Copy link
Member

xukai92 commented Nov 13, 2017

List of un-fixed warnings (taken from https://travis-ci.org/yebai/Turing.jl/jobs/301057755):

  • floor
  • tanh
  • abs
  • logpdf
  • log

@@ -83,7 +83,7 @@ assume{T<:Distribution}(spl::Void, dists::Vector{T}, vn::VarName, var::Any, vi::
end
end

acclogp!(vi, sum(logpdf(dist, rs, istrans(vi, vns[1]))))
acclogp!(vi, sum(logpdf_with_trans(dist, rs, istrans(vi, vns[1]))))
Copy link
Member

Choose a reason for hiding this comment

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

@yebai Plz have a look at this change - do you agree what we had before was a bug?

Copy link
Member

Choose a reason for hiding this comment

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

@yebai This is actually (mathematically) correct ... as there is another bug - we didn't use customised init here for vectorised assume at all. I will push another commit to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - it looks correct to me.

@yebai yebai merged commit 3f788de into master Nov 16, 2017
@yebai yebai deleted the Fix-173 branch November 16, 2017 14:58
emilemathieu pushed a commit that referenced this pull request Nov 17, 2017
* Initial support for infix ~ (#173).

* seperate model wrapper call in sample.jl test

* update ForwardDiff.Dual signature

* sample.jl test passed

* fix some tests for Julia 0.6

* remove typealias for 0.6

* make some functions 0.6-ish

* make abstract 0.6-ish

* change some decpreated functions

* Update .travis.yml

* Update appveyor.yml

* Update appveyor.yml

* fix test

* Deprecations on package loading fixed

* fix deprecations

* implement callbacks for inner function

* fix model type bug

* Fix type

* update Dual in benchmark

* update Dual constructor

* Bump up required Julia version to 0.6

* Disable depreciated warning messages for `consume/produce`.

* Remove duplicate definition of produce.

* fix floor, tanh, abs, log

* fix logpdf warning and bug

* fix vec assume init

* Travis: Allow `Benchmarking` test to fail.
yebai added a commit that referenced this pull request Sep 18, 2018
* Initial support for infix ~ (#173).

* seperate model wrapper call in sample.jl test

* update ForwardDiff.Dual signature

* sample.jl test passed

* fix some tests for Julia 0.6

* remove typealias for 0.6

* make some functions 0.6-ish

* make abstract 0.6-ish

* change some decpreated functions

* Update .travis.yml

* Update appveyor.yml

* Update appveyor.yml

* fix test

* Deprecations on package loading fixed

* fix deprecations

* implement callbacks for inner function

* fix model type bug

* Fix type

* update Dual in benchmark

* update Dual constructor

* Bump up required Julia version to 0.6

* Disable depreciated warning messages for `consume/produce`.

* Remove duplicate definition of produce.

* fix floor, tanh, abs, log

* fix logpdf warning and bug

* fix vec assume init

* Travis: Allow `Benchmarking` test to fail.
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.

2 participants