-
Notifications
You must be signed in to change notification settings - Fork 32
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
(Maybe) Find a way to avoid initialising with undef values #784
Comments
We may not want to automate this. The current behaviour is standard Julia syntax: users must initialise vectors before using them. However, we could make the error message more user-friendly. |
I agree, and things would get even worse for models with multiple predictors. I am quite torn about the new fitting syntax with A mix of both worlds would be idea, i.e.,
The model "expects" In any case, leaving it as-is, even with a clearer error message, will likely limit a lot of functionalities previously possible so it would be an unfortunate regression |
I do see @yebai's point too, he's correct that x isn't passed as an argument to the function, so it's not in scope. @DominiqueMakowski There's much I like about your suggestion. There're a couple of reasons why I'm not inclined towards doing that though. One is pragmatic: if we have models with multiple positional arguments, then we should expect the following to be the same, @model function f(x, y) ... end
f() | (x = x0, y = y0)
f() | (y = y0, x = x0) but there isn't any way to accomplish this without parsing the names of the model arguments, which feels too fragile for me to be comfortable with. Secondly, just in principle, I'm kind of uncomfortable with a function signature f(x, y) and an invocation f(), because that's just a different kind of 'oddity' that must be explained to users, leaving us in a similar situation to what it is now. Anyway, I was thinking there were a couple of useful things we could do:
In any case, the old pass-it-as-a-function-argument style probably won't be removed anytime soon. Personally I have quite a few reservations re. the new syntax because there are also correctness issues with it (e.g. #702 (comment)) and on balance, I think that if I were a user, I'd prefer sticking to the old syntax, at least for now. That's not to say that any of this is useless or bad. I think that all of these discussions and considerations are really valuable things that will guide us in settling on a final syntax that we're all happy with. I've edited the issue title just to capture all of our uncertainty with this. I think it's worth looking into, but definitely not something that we want to implement and push out asap. |
Some misconceptions about the @model function f(N)
x = Vector{Float64}(undef, N)
for i in 1:length(x)
x[i] ~ Normal()
end
return x
end
model = f(2)
model | (x=[1.0, 2.0],) Here, the function |
Re-raising the issue in TuringLang/Turing.jl#2477, which boils down to the following:
The issue here is sort of twofold:
length(x)
,x
doesn't exist. This is the immediate cause of the above error.x
doesn't exist and you can't assign tox[1]
if it doesn't exist.Both of these can be circumvented by explicitly initialising
x
:However, this is both confusing (it requires understanding of the DynamicPPL compiler to explain why this happens), rather ugly, and also prone to errors / other restrictions because the length of
x
has to be specified inside the model. It would be nice if there was a way to avoid the need for this.This would require a lot of work, though, and some very careful consideration to make sure that any implementation has both correct and predictable behaviour.
The text was updated successfully, but these errors were encountered: