jonniedie / ComponentArrays.jl

Arrays with arbitrarily nested named components.
MIT License
291 stars 35 forks source link

[WIP] Merge ComponentVectors #217

Open Qfl3x opened 1 year ago

Qfl3x commented 1 year ago

Issue #186

Implemented merge on 2 (or more) ComponentVectors, similar to merge on NamedTuples or Dicts. With docs.

Qfl3x commented 1 year ago

This PR will break precompilation. Working on using Macros instead of the Eval (bad idea)

Qfl3x commented 1 year ago

I think it's resolved now.

jonniediegelman commented 1 year ago

I'm going to go ahead and reopen this because I think it will be useful to have. If you get a chance to change it and write some tests for it, I'd appreciate it. If not, I can do it at some point hopefully soon.

codecov-commenter commented 1 year ago

Codecov Report

Merging #217 (6326604) into main (59031c1) will decrease coverage by 1.70%. Report is 1 commits behind head on main. The diff coverage is 0.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
- Coverage   72.75%   71.06%   -1.70%     
==========================================
  Files          20       21       +1     
  Lines         690      698       +8     
==========================================
- Hits          502      496       -6     
- Misses        188      202      +14     
Files Changed Coverage Δ
src/ComponentArrays.jl 100.00% <ø> (ø)
src/componentarray.jl 71.12% <0.00%> (-7.78%) :arrow_down:

... and 7 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

:loudspeaker: Have feedback on the report? Share it here.

Qfl3x commented 1 year ago

The whole point was to make it differentiable via ChainRules, though this isn't. A solution exists for making it work with ChainRules, however, I think focusing on #207 is best.

Qfl3x commented 1 year ago

Last two commits added type promotion.

jonniedie commented 1 year ago

Ah, I see. Yeah, we might want to make this work with merge directly then because ChainRules doesn't support differentiating through keyword arguments. We'll likely need to do something similar to your first approach, but with @generated functions for type-stability.

Qfl3x commented 1 year ago

I think something similar to what @vboussange did could work:

import Base
function Base.merge(ca::ComponentArray{T}, ca2::ComponentArray{T}) where T
    ax = getaxes(ca)
    ax2 = getaxes(ca2)
    vks = valkeys(ax[1])
    vks2 = valkeys(ax2[1])
    _p = Vector{T}()
    for vk in vks
        if vk in vks2
            _p = vcat(_p, ca2[vk])
        else
            _p = vcat(_p, ca[vk])
        end
    end
    ComponentArray(_p, ax)
end

Though this one implies that ca2 is a subset of ca. So I'm working on "generalizing" it a bit further. It all comes down to a "smart" Axis merge.

Qfl3x commented 1 year ago

The last commit has an example that might be expanded on. Though it's a bit convoluted, it does work.

vancleve commented 4 months ago

Is this still necessary for getting Zygote to work with ComponentArrays sensu https://github.com/jonniedie/ComponentArrays.jl/issues/207?