Skip to content

Improvements to VarNamedVector #1098

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Oct 29, 2025
Merged

Improvements to VarNamedVector #1098

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
150de71
Change VNV to use Dict rather than OrderedDict
mhauru Oct 28, 2025
30ac1d0
Change concretisation from map(identity, x) to a comprehension
mhauru Oct 28, 2025
4ae0c6d
Improve tighten_types!! and loosen_types!!
mhauru Oct 28, 2025
4c8b006
Fix use of set_transformed!!
mhauru Oct 28, 2025
c8b0b88
Fix push!! for VarInfos
mhauru Oct 28, 2025
1f7152b
Change the default element types in VNV to be Union{}
mhauru Oct 28, 2025
61c96b0
In untyped_vector_varinfo, don't rely on Metadata
mhauru Oct 28, 2025
2a10be9
Code style
mhauru Oct 28, 2025
bb83d93
Run formatter
mhauru Oct 29, 2025
99a7d32
In VNV, use typejoin rather than promote_type
mhauru Oct 29, 2025
513edc5
Merge branch 'mhauru/varnamedvector-speed' into mhauru/vnv-speed2
mhauru Oct 29, 2025
d30eca8
Bump patch version to 0.38.4
mhauru Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/contexts/init.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,9 @@ function tilde_assume!!(
end
# Neither of these set the `trans` flag so we have to do it manually if
# necessary.
insert_transformed_value && set_transformed!!(vi, true, vn)
if insert_transformed_value
vi = set_transformed!!(vi, true, vn)
end
# `accumulate_assume!!` wants untransformed values as the second argument.
vi = accumulate_assume!!(vi, x, logjac, vn, dist)
# We always return the untransformed value here, as that will determine
Expand Down
1 change: 1 addition & 0 deletions src/simple_varinfo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ function set_transformed!!(vi::SimpleOrThreadSafeSimple, trans::Bool, ::VarName)
"Individual variables in SimpleVarInfo cannot have different `set_transformed` statuses.",
)
end
return vi
end

is_transformed(vi::SimpleVarInfo) = !(vi.transformation isa NoTransformation)
Expand Down
12 changes: 9 additions & 3 deletions src/varinfo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,18 +789,24 @@ function setval!(md::Metadata, val, vn::VarName)
return md.vals[getrange(md, vn)] = tovec(val)
end

function set_transformed!!(vi::NTVarInfo, val::Bool, vn::VarName)
md = set_transformed!!(getmetadata(vi, vn), val, vn)
return Accessors.@set vi.metadata[getsym(vn)] = md
end

function set_transformed!!(vi::VarInfo, val::Bool, vn::VarName)
set_transformed!!(getmetadata(vi, vn), val, vn)
return vi
md = set_transformed!!(getmetadata(vi, vn), val, vn)
return VarInfo(md, vi.accs)
end

function set_transformed!!(metadata::Metadata, val::Bool, vn::VarName)
metadata.is_transformed[getidx(metadata, vn)] = val
return metadata
end

function set_transformed!!(vi::VarInfo, val::Bool)
for vn in keys(vi)
set_transformed!!(vi, val, vn)
vi = set_transformed!!(vi, val, vn)
end
Comment on lines 807 to 810
Copy link
Member

@penelopeysm penelopeysm Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how Julia works on this front, but is it ever possible that changing what vi is bound to might invalidate the iteration over keys(vi)? I think probably not, but ...??

Copy link
Member

@penelopeysm penelopeysm Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's not so much rebinding vi that's the issue, but if set_transformed!! were to mutate vi, then that might be problematic. I suppose it can't mutate it in a way such that keys(vi) would result in something different, so this probably can't error. But I'm a bit paranoid about this kind of code structure. Obviously, this predates this PR.

Copy link
Member Author

@mhauru mhauru Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, the keys(vi) call I think is evaluated once when the loop is started, and it'll keep its reference to the original vi even if the name vi is then bound to something else. Like in

julia> d = Dict(:a => 1)
Dict{Symbol, Int64} with 1 entry:
  :a => 1

julia> ks = keys(d)
KeySet for a Dict{Symbol, Int64} with 1 entry. Keys:
  :a

julia> d = Dict(:b => 1)
Dict{Symbol, Int64} with 1 entry:
  :b => 1

julia> collect(ks)
1-element Vector{Symbol}:
 :a

keys(vi) is a lazy iterator though, so what I would feel dicey about is changing the set of keys in vi mid-iteration. This is a thing:

julia> d = Dict(:a => 1)
Dict{Symbol, Int64} with 1 entry:
  :a => 1

julia> ks = keys(d)
KeySet for a Dict{Symbol, Int64} with 1 entry. Keys:
  :a

julia> d[:b] = 2
2

julia> collect(ks)
2-element Vector{Symbol}:
 :a
 :b

which can result in weirdness:

julia> d = Dict{Any,Int}(:a => 1, :b => 2)
Dict{Any, Int64} with 2 entries:
  :a => 1
  :b => 2

julia> for k in keys(d)
           d[repr(k)] = d[k]
       end

julia> d
Dict{Any, Int64} with 7 entries:
  :a               => 1
  :b               => 2
  "\":b\""         => 2
  ":b"             => 2
  "\"\\\":a\\\"\"" => 1
  ":a"             => 1
  "\":a\""         => 1

Copy link
Member

@penelopeysm penelopeysm Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think from a programming perspective, maybe the safest way would be to do a version of keys(vi) that's eagerly evaluated, maybe collect(keys(vi))?

But also from a human perspective it's quite easy to see that keys(vi) can't be changed, so also happy to let it slide.

Copy link
Member Author

@mhauru mhauru Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep it as is, because I'm pretty convinced it's okay (and not due to an implementation detail, but because set_transformed!! has no business touching the keys), and lazy iterators are nice. Could add the call to collect if it bothers you.

Copy link
Member

@penelopeysm penelopeysm Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't bother me enough to want to do it.


return vi
Expand Down