[Merged by Bors] - Fix concretization for varname
#75
Conversation
|
bors try |
|
This won't fix the DPPL tests alone. The PR causing these fundamentally changes how we treat |
| return first_subsumed && subsumes_indices(Base.tail(t1), Base.tail(t2)) | ||
| end | ||
|
|
||
| subsumes_index(i::Colon, ::Colon) = error("Colons cannot be subsumed") |
There was a problem hiding this comment.
Thanks for the investigation; can you list an example of how/where this is used in DynamicPPL?
There was a problem hiding this comment.
It affects every model which uses : for indexing.
There was a problem hiding this comment.
Because we then try to call subsumes on it when checking.
There was a problem hiding this comment.
I remember these issues from the latent Dirichlet allocation model and MCMCChains; IMO, we should always replace : with 1:N where N is a constant.
There was a problem hiding this comment.
Yeah, but this issue goes a bit deeper than that.
The PR causing these issues were trying to address things like OffsetArrays.jl not working properly, etc. So I think we're now also in need of concretization stuff like x[2:3], etc. I'm trying to figure this out.
|
bors r+ |
Use `Setfield.need_dynamic_lens` as the default value for `concretize` in `varname`. This way we preserve pre-0.6 behavior, thus not make the 0.6-release "less" breaking. Ref: TuringLang/DynamicPPL.jl#440 Co-authored-by: Hong Ge <[email protected]>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
varnamevarname
Use
Setfield.need_dynamic_lensas the default value forconcretizeinvarname. This way we preserve pre-0.6 behavior, thus not make the 0.6-release "less" breaking.Ref: TuringLang/DynamicPPL.jl#440