Refactor VarName #50
Merged
Refactor VarName #50
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devmotion
reviewed
Mar 22, 2020
phipsgabler
commented
Mar 22, 2020
phipsgabler
commented
Mar 22, 2020
devmotion
reviewed
Mar 22, 2020
This comment has been minimized.
This comment has been minimized.
|
I'm now getting a couple of equal errors from ForwardDiff. Any ideas? |
phipsgabler
commented
Mar 23, 2020
devmotion
reviewed
Mar 23, 2020
|
The logs indicate that the error is caused by DynamicPPL.jl/test/Turing/core/ad.jl Line 102 in 8028aaa length(θ) = 0. So there's an empty array passed to gradient_logp?
|
|
If that is actually the reason for the errors, then it might be caused by |
phipsgabler
changed the title
|
I just missed updating of the |
|
I updated some more tests, and am now getting the following error from this line: |
|
I'd suggest printing the names of the chain that was constructed in the step before by running |
devmotion
reviewed
Apr 3, 2020
devmotion
approved these changes
Apr 4, 2020
devmotion
reviewed
Apr 6, 2020
mohamed82008
reviewed
Apr 6, 2020
mohamed82008
reviewed
Apr 6, 2020
mohamed82008
reviewed
Apr 6, 2020
|
|
||
| """ | ||
| @varname(var) | ||
| subsumes(u::VarName, v::VarName) |
There was a problem hiding this comment.
Where is this function used/needed?
There was a problem hiding this comment.
It replaces the former string matching in in(::VarName, ::Tuple), and is used in inspace. Like, checking that x[1][2] is in a space containing x[:] or something. If you have a better name than subsumes, I'd be happy to rename it!
There was a problem hiding this comment.
I see, but I don't see this function used very much here. So I was wondering if you have another use case for it. Most of the time we only need to check if a VarName's symbol is in a tuple of symbols.
There was a problem hiding this comment.
If spaces were allowed to contain just symbols, inspace would be trivial indeed. But since it also allowes indexed VarNames, there must be a way to determine compatibility. Like what the string matching in the previous _in function did.
mohamed82008
reviewed
Apr 6, 2020
| Check whether `vn`'s variable symbol is in `space`. | ||
| """ | ||
| inspace(vn, space::Tuple{}) = true # empty space is treated as universal set | ||
| inspace(vn, space::Tuple) = vn in space |
There was a problem hiding this comment.
Shouldn't this use the symbol of vn?
There was a problem hiding this comment.
Those two are the cases when vn is not a VarName. I left those because I think some parts are still not consistently using VarName. It is consistent with the old usage of in, which only added the special check for VarName.
|
@phipsgabler Can you add the fix in #61 (comment) (i.e., basically add |
|
@devmotion Sure. What would be a good test? I somehow don't get the following to work: @model testmodel(z) = begin
m ~ Normal()
z[1:end] ~ MvNormal(fill(m, length(z)), 1.0)
end
model = testmodel()
@test all(z -> isapprox(z, 0; atol = 0.1), mean(model() for _ in 1:1000))(Kind of embarrassing that I still don't really know how to actually work with Turing...) |
|
Oh you have to provide |
|
OK, I think I'm finished here. |
|
Good work @phipsgabler! |
|
Really good, it's great we do not work with strings anymore internally 👍 |
|
MCMCChains still does, somehow, I think... |
|
It needs TuringLang/MCMCChains.jl#197, that should solve most String/Symbol issues IIRC. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the changes discussed in #49: make
VarNameuse a tuple of tuples instead of a string for the indexing value. For that, I mostly reused the existing implementations of@vindsand@vsym.While at that, I also refactored the methods realing to
VarNamea bit and moved some leftovers to their right place invarname.jl.Updating all the rest of the code base and the tests is still to be done.