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

(Maybe) Find a way to avoid initialising with undef values #784

Open
penelopeysm opened this issue Jan 23, 2025 · 4 comments
Open

(Maybe) Find a way to avoid initialising with undef values #784

penelopeysm opened this issue Jan 23, 2025 · 4 comments
Labels
enhancement New feature or request

Comments

@penelopeysm
Copy link
Member

penelopeysm commented Jan 23, 2025

Re-raising the issue in TuringLang/Turing.jl#2477, which boils down to the following:

using DynamicPPL, Distributions

@model function f()
    for i in 1:length(x)
        x[i] ~ Normal()
    end
    return x
end

model = f() | (x=[1.0, 2.0],)
model()
# ERROR: UndefVarError: `x` not defined in `Main`

The issue here is sort of twofold:

  1. Things on the lhs of tilde are not brought into scope until after the tilde statement has been executed. Thus, in the call to length(x), x doesn't exist. This is the immediate cause of the above error.
  2. The lhs of the tilde statements above can't be assigned to anyway, because x doesn't exist and you can't assign to x[1] if it doesn't exist.

Both of these can be circumvented by explicitly initialising x:

using DynamicPPL, Distributions

@model function f()
    x = Vector{Float64}(undef, 2)
    for i in 1:length(x)
        x[i] ~ Normal()
    end
    return x
end

model = f() | (x=[1.0, 2.0],)
model()
# 2-element Vector{Float64}:
# 1.0
# 2.0

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.

@yebai
Copy link
Member

yebai commented Jan 23, 2025

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.

@DominiqueMakowski
Copy link

DominiqueMakowski commented Jan 23, 2025

this is both confusing, rather ugly, and also prone to errors / other restrictions

I agree, and things would get even worse for models with multiple predictors.
As a side comparison, one of the biggest and most well-received syntactic update of RxInfer was to drop the need for datavar() declaration at the beginning of the model... so introducing something similar in Turing would indeed feel like a big step backwards...

I am quite torn about the new fitting syntax with |, one the one hand I appreciate its mathematical authenticity, on the other I find it confusing for standard programmers because "how can I define function that uses a variable that is not passed as an argument" (it does seem very strange for outsiders and people not familiar with macros and Julia's inner workings).

A mix of both worlds would be idea, i.e.,

@model function f(x)
    for i in 1:length(x)
        x[i] ~ Normal()
    end
    return x
end

model = f() | (x=[1.0, 2.0],)

The model "expects" x upon definition (programmers are happy), and it is provided via the rhs of | (Bayesianists are in love)

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

@penelopeysm
Copy link
Member Author

penelopeysm commented Jan 23, 2025

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:

  1. More informative error message + docs. The 'problem' with this is that we would have to wrap the entire model evaluation function inside a gigantic try/catch so that we can catch UndefVarError and check whether it's a variable that was conditioned on, and I don't know what the performance implications of that are, but I fear it might not be good. This fear might be unfounded, the only way to find out is to... try (ha ha).

  2. A permanent 'fix' would be to modify the model evaluation function to include a block at the top where any variables conditioned upon are initialised to their respective values. This is definitely possible with the macro based system we have but it would be really important to discuss this thoroughly, because anything done with the macro system is just more hidden magic that has the potential to confuse.

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.

@penelopeysm penelopeysm changed the title Find a way to avoid initialising with undef values (Maybe) Find a way to avoid initialising with undef values Jan 23, 2025
@yebai
Copy link
Member

yebai commented Jan 24, 2025

Some misconceptions about the condition / fix semantics exist. condition/fix acts on models rather than the model constructor functions. For a concrete example:

@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 f is a model constructor, while model is a model instance. The condition operation is performed on model rather than f. To construct a model model, the statement model = f(2) needs to run successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants