feat: implement delete!! for VarNamedTuple (#1239)

[ArpanC6]

Closes #1239

What was the problem?

VarNamedTuple had no delete!! method, unlike other collection types in Julia which typically support deletion.

What did I do?

Implemented BangBang.delete!! for VarNamedTuple in src/varnamedtuple/map.jl. The implementation filters out the given VarName (and any names subsumed by it) using the existing subset and keys functions:

function BangBang.delete!!(vnt::VarNamedTuple, vn::VarName)
    remaining_vns = filter(k -> !subsumes(vn, k), keys(vnt))
    return DynamicPPL.subset(vnt, remaining_vns)
end

Tests

All 62,988 existing tests pass locally.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.37%. Comparing base (10a3651) to head (bd9a7ac).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1381      +/-   ##
==========================================
+ Coverage   82.35%   82.37%   +0.01%     
==========================================
  Files          50       50              
  Lines        3531     3534       +3     
==========================================
+ Hits         2908     2911       +3     
  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.

[ArpanC6]

Hlw @penelopeysm I reviewed the issue and saw that VarNamedTuple was missing a delete!! method. To address this I implemented the BangBang.delete!! method by filtering out the provided VarName using the existing subset and keys functions. I also wrote tests to cover this new functionality. All the necessary CI checks have passed and codecov shows 100% coverage for the changes. When you have a moment could you take a look? Thanks

[penelopeysm]

Thanks for the PR!

While this implementation is correct in most cases, there are some cases where it will fail.

1. The subsumes check is a bit conservative and will fail to detect some things, which are described in http://turinglang.org/AbstractPPL.jl/stable/varname/#AbstractPPL.subsumes.

2. Secondly, it doesn't allow you to delete part of a variable. Consider this:

julia> v = @vnt begin
           x := [1, 2]
       end
VarNamedTuple
└─ x => [1, 2]

julia> delete!!(v, @varname(x[1])) # what should this return?

Right now, this implementation of delete!! will do nothing, because we have that

julia> keys(v)
1-element Vector{VarName}:
 x

and x[1] doesn't subsume x. I think it would be very reasonable to expect the call to delete!! to return something like this, where the vector has been converted into a PartialArray:

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

Apart from this, it is also pretty inefficient, because it has to traverse the entire VarNamedTuple to figure out which keys should remain, and then rebuild a complete VarNamedTuple that doesn't contain it. (Look at the implementation of subset: it's a loop over the keys which calls templated_setindex!!.)

This means that if you have a VNT that has 26 keys, and you remove one of them, you have to call templated_setindex!! 25 times. And this is probably unnecessary. VNT has a tree-like structure, which should allow you to identify exactly which branches can be pruned when you call delete!!, and not have to touch any of the remaining ones. For example, look at the way setindex!! is implemented. It uses the VarName to recurse into the correct part that needs to be changed, and doesn't touch anything else. The implementation here should be more surgical like that.

It's not going to be easy to write correctly, and I can only apologise, but it's unlikely that I can review it.

[penelopeysm]

These tests are a good starting point, but they are still too simple and only investigate the happy path where things are easy to make work. Notice how thorough the tests for get/set are above. Deletion is less important than those operations, so maybe the tests for deletion don't have to live up to that standard, but it needs to be much closer to them.

[ArpanC6]

Thank you for the detailed feedback @penelopeysm I understand the limitations you've pointed out. I'll study the VarNamedTuple internals more carefully before attempting a more surgical implementation. I may close this PR for now and reopen once I have a better understanding.