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

[WIP] Merge ComponentVectors #217

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[WIP] Merge ComponentVectors #217

wants to merge 7 commits into from

Conversation

Qfl3x
Copy link

@Qfl3x Qfl3x commented Sep 6, 2023

Issue #186

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

@Qfl3x Qfl3x changed the title Merge ComponentVectors [WIP] Merge ComponentVectors Sep 7, 2023
@Qfl3x
Copy link
Author

Qfl3x commented Sep 7, 2023

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

@Qfl3x
Copy link
Author

Qfl3x commented Sep 7, 2023

I think it's resolved now.

@Qfl3x Qfl3x closed this Sep 7, 2023
@jonniediegelman
Copy link
Collaborator

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.

Copy link
Collaborator

@jonniediegelman jonniediegelman left a comment

Choose a reason for hiding this comment

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

It might be good to have this in as

Base.merge(ca::ComponentVector) = ca
Base.merge(ca1::ComponentVector, ca2::ComponentVector) = ComponentVector(ca2; ca1...)
Base.merge(ca1::ComponentVector, ca2::ComponentVector, others...) = merge(merge(ca1, ca2), others...)

Because the ComponentVector(ca2; ca1...) syntax can already perform the merge via destructuring and splatting ca1.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

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%.

❗ 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%) ⬇️

... and 7 files with indirect coverage changes

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

📢 Have feedback on the report? Share it here.

@SciML SciML deleted a comment from jonniediegelman Sep 9, 2023
@Qfl3x
Copy link
Author

Qfl3x commented Sep 11, 2023

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
Copy link
Author

Qfl3x commented Sep 11, 2023

Last two commits added type promotion.

@jonniedie
Copy link
Collaborator

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
Copy link
Author

Qfl3x commented Sep 15, 2023

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
Copy link
Author

Qfl3x commented Sep 15, 2023

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

@vancleve
Copy link

Is this still necessary for getting Zygote to work with ComponentArrays sensu #207?

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

Successfully merging this pull request may close these issues.

5 participants