improve getindex error message

[hardikxk]

closes #994

[penelopeysm]

Thanks @hardik-xi11 for this! Before we proceed, I think it might be worth checking whether the original issue is actually still an issue. The internal data structure of VarInfo was changed substantially in v0.40 and so I don't think the error message in the original issue is exactly the same anymore. I get this on the current version:

julia> DynamicPPL.getindex_internal(v, @varname(y))
ERROR: type NamedTuple has no field y
Stacktrace:
 [1] getindex
   @ ./namedtuple.jl:168 [inlined]
 [2] _getindex_optic(vnt::VarNamedTuple{(:x,), Tuple{…}}, optic::AbstractPPL.Property{:y, AbstractPPL.Iden})
   @ DynamicPPL.VarNamedTuples ~/ppl/dppl/src/varnamedtuple/getset.jl:37
 [3] _getindex_optic(vnt::VarNamedTuple{(:x,), Tuple{TransformedValue{…}}}, vn::VarName{:y, AbstractPPL.Iden})
   @ DynamicPPL.VarNamedTuples ~/ppl/dppl/src/varnamedtuple/getset.jl:32
 [4] getindex(vnt::VarNamedTuple{(:x,), Tuple{TransformedValue{Vector{Float64}, Unlink}}}, vn::VarName{:y, AbstractPPL.Iden})
   @ DynamicPPL.VarNamedTuples ~/ppl/dppl/src/varnamedtuple/vnt.jl:90
 [5] getindex_internal(vi::VarInfo{…}, vn::VarName{…})
   @ DynamicPPL ~/ppl/dppl/src/varinfo.jl:380
 [6] top-level scope
   @ REPL[3]:1
Some type information was truncated. Use `show(err)` to see complete types.

There's also an argument that things like getindex_internal and get_transformed_value aren't really very public functions and so it might be more okay to have a less intuitive error message.

Now, I do actually still think that the error message above can be improved. However, the fix for this would be more targeted and at a lower level. In particular, notice that this is the same error that you get when running

julia> using DynamicPPL; vnt = @vnt begin
           x := 1
       end
VarNamedTuple
└─ x => 1

julia> vnt[@varname(y)]
ERROR: type NamedTuple has no field y
Stacktrace:
 [1] getindex
   @ ./namedtuple.jl:168 [inlined]
 [2] _getindex_optic(vnt::VarNamedTuple{(:x,), Tuple{Int64}}, optic::AbstractPPL.Property{:y, AbstractPPL.Iden})
   @ DynamicPPL.VarNamedTuples ~/ppl/dppl/src/varnamedtuple/getset.jl:37
 [3] _getindex_optic(vnt::VarNamedTuple{(:x,), Tuple{Int64}}, vn::VarName{:y, AbstractPPL.Iden})
   @ DynamicPPL.VarNamedTuples ~/ppl/dppl/src/varnamedtuple/getset.jl:32
 [4] getindex(vnt::VarNamedTuple{(:x,), Tuple{Int64}}, vn::VarName{:y, AbstractPPL.Iden})
   @ DynamicPPL.VarNamedTuples ~/ppl/dppl/src/varnamedtuple/vnt.jl:90
 [5] top-level scope
   @ REPL[6]:1

So I think it would be more targeted to improve the getindex on VarNamedTuple here. In particular, if we could throw a KeyError(vn) then that would be semantically useful as well (rather than a generic error(...)). What do you think?

DynamicPPL.jl/src/varnamedtuple/vnt.jl

Lines 89 to 92 in 81a245a

	function Base.getindex(vnt::VarNamedTuple, vn::VarName)
	result = _getindex_optic(vnt, vn)
	return unwrap_internal_array(result)
	end

[penelopeysm]

(There are some subtle design choices to be made here, though: I just discovered this sort of thing

julia> vnt = @vnt begin
           x := 1
       end
VarNamedTuple
└─ x => 1

julia> vnt[@varname(x[1])]
1

julia> vnt[@varname(x[2])]
ERROR: BoundsError: attempt to access Int64 at index [2]
[...]

For x[1], this is arguably right because indexing into an integer i with i[1] gives the integer back.

julia> c = 3
3

julia> c[1]
3

However, that indexing behaviour is not defined or controlled by VarNamedTuple, but rather Base.

That means that for x[2] the BoundsError is arguably the correct error to throw, because the error comes from a failure to use Base's indexing semantics, and has nothing to do with VNT's indexing semantics. One could argue that it would be wrong to emit KeyError in such a case.

It is complicated...)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.28%. Comparing base (b854e0d) to head (fbb24dd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1367      +/-   ##
==========================================
+ Coverage   82.26%   82.28%   +0.02%     
==========================================
  Files          49       49              
  Lines        3512     3516       +4     
==========================================
+ Hits         2889     2893       +4     
  Misses        623      623              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hardikxk]

Right for scalars the BoundsError is the natural behaviour . I've added the necessary changes

[penelopeysm]

Thank you! I'm afraid I'm going to be a bit nitpicky, but this doesn't cover all cases. Consider:

julia> using DynamicPPL; vnt = @vnt begin
           x.a := 1.0
       end
VarNamedTuple
└─ x => VarNamedTuple
        └─ a => 1.0

julia> keys(vnt)
1-element Vector{VarName}:
 x.a

julia> vnt[@varname(x.b)]
ERROR: type NamedTuple has no field b
Stacktrace:
 [1] getindex
   @ ./namedtuple.jl:168 [inlined]

Now, you could insert a generic check like haskey(vnt, vn) || throw(KeyError(...)). But that's bad for performance because you have to check the structure once to see if there's a key, and once to actually get the key. I think that the "correct" way to do it would be to

• add an extra parameter into _getindex_optic which specifies the "original" VarName the user wanted. We have to pass this in because _getindex_optic recurses into itself and when you do so, you won't have information about the original VarName.
• at each possible point where it can raise an error, instead throw a KeyError.

Does that sound reasonable?

[penelopeysm]

Ah, and I shouldn't have said "at each possible point", because the design question that was discussed above, basically amounts to deciding which lines in _getindex_optic should throw a KeyError, and which places should just throw the original error.

[penelopeysm]

BTW, if (in your judgment) the benefit of fixing the error message is not really worth the time / energy needed to wrestle with these thorny questions, or if you would rather try to fix something else / do something else with your time, that is also a totally valid outcome. We could close the original issue as a wontfix: that would be a perfectly legitimate technical decision -- just as legitimate as fixing it would be. As you can tell, I haven't spent my time on this either :-)

[hardikxk]

Yup the issue could be closed or otherwise i have implemented your suggested changes to modify the get_index_optic methods in getset.jl

function _getindex_optic(vnt::VarNamedTuple, vn::VarName)
    return _getindex_optic(vnt, AbstractPPL.varname_to_optic(vn), vn)
end

@inline _getindex_optic(x::Any, o::AbstractPPL.AbstractOptic) = _getindex_optic(x, o, nothing)

@inline _getindex_optic(@nospecialize(x::Any), ::AbstractPPL.Iden, orig_vn) = x
@inline _getindex_optic(x::Any, o::AbstractPPL.AbstractOptic, orig_vn) = o(x)
function _getindex_optic(vnt::VarNamedTuple, optic::AbstractPPL.Property{S}, orig_vn) where {S}
    if !haskey(vnt.data, S)
        orig_vn !== nothing && throw(KeyError(orig_vn))
    end
    return _getindex_optic(getindex(vnt.data, S), optic.child, orig_vn)
end

function _getindex_optic(pa::PartialArray, optic::AbstractPPL.Index, orig_vn)
...
    return _getindex_optic(child_value, optic.child, orig_vn)

but then the logdensityfunction tests fail which could be fixed by refactoring it.

@test_throws KeyError init!!(     # this instead of ErrorException
...
)

This could be looked at as redundant just to make an error look more pretty or if these changes would be good then i can replace the old vnt.jl condition.

[penelopeysm]

Sorry for the delay! I think that looks pretty much correct. It'd fine to change the logdensityfunction.jl tests accordingly since after all that is the entire purpose of this PR, to have a more meaningful error message.

Before launching in, maybe another part that one might ask about is the PartialArray sections:

julia> v = @vnt begin
           @template x = zeros(2)
           x[1] := 1.0
       end
VarNamedTuple
└─ x => PartialArray size=(2,) data::Vector{Float64}
        └─ (1,) => 1.0

julia> v[@varname(x[2])]
ERROR: BoundsError: attempt to access DynamicPPL.VarNamedTuples.PartialArray{Float64, 1, Vector{Float64}, Vector{Bool}} at index [2, Base.Pairs{Symbol, Union{}, Tuple{}, @NamedTuple{}}()]

julia> v[@varname(x)]
ERROR: ArgumentError: Cannot extract data from PartialArray because some elements are not set.

The same happens with ArrayLikeBlocks (https://turinglang.org/DynamicPPL.jl/stable/vnt/arraylikeblocks/):

julia> v = @vnt begin
           @template x = zeros(5)
           x[1:2] := missing
       end
VarNamedTuple
└─ x => PartialArray size=(5,) data::Vector{DynamicPPL.VarNamedTuples.ArrayLikeBlock{Missing, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}}
        ├─ (1,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Missing, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(missing, (1:2,), NamedTuple(), (2,))
        └─ (2,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Missing, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(missing, (1:2,), NamedTuple(), (2,))

julia> v[@varname(x[1])]
ERROR: ArgumentError: A non-Array value set with a range of indices must be retrieved with the same
range of indices.

All of these throw different errors. Should they also be KeyErrors? I feel a little bit less convinced in this case personally -- the annoying thing about KeyError is that you can't attach a message with them to say why there wasn't this key. So moving to KeyError would actually in a way be a loss of information.

[hardikxk]

The PartialArrays and ArrayLikeBlocks are correct as they are right now IMO.

So moving to KeyError would actually in a way be a loss of information.

I agree with this so only specifically targeting Property for now is correct.

and no need for sorry I get we are all busy :)

[penelopeysm]

Thanks! I think this mostly looks sensible. However, I'd like to push a bit on _getindex_optic. I looked through the codebase a bit, and my suspicion is that every two-argument call site can be switched over to use the three-argument version. Is there any place where we still need the two-argument version?

I ask because I think it's just confusing for the third argument to sometimes be a real VarName and sometimes be nothing. I kind of think it should always be a real VarName.

[hardikxk]

thanks for the review, went through it myself as well and yea makes sense to have it as VarName.

[penelopeysm]

Thanks, just a docstring touchup!