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

CVIProjection with constraints on interfaces #428

Merged
merged 20 commits into from
Nov 14, 2024
Merged

Conversation

Nimrais
Copy link
Member

@Nimrais Nimrais commented Nov 6, 2024

Added flexibility to projection forms in CVI rules

Changes

Enhanced CVIProjection with projection forms control

  • Added out_prjparams parameter to control output edge projection form
  • Added in_prjparams parameter as a NamedTuple to control input edge projection forms

Added flexible form selection mechanism

  • Created get_kth_in_form to safely access input forms by index
  • Forms are optional - if not specified, the system uses the default form inferred from the marginal
  • Added smart form matching to maintain optimal performance:
    • Uses efficient supplementary distribution when forms match or default form is used
    • Falls back to including logpdf in objective only when forms truly differ

Optimized projection logic

  • For output edge:
    • Uses specified form if provided
    • Falls back to inferred form from marginal if not specified
  • For input edges:
    • Allows per-edge form specification through NamedTuple
    • Maintains optimal performance by detecting form compatibility
    • Handles form mismatches gracefully by incorporating logpdf in objective

Example Usage

# Specify output form
form_out = ProjectedTo(Normal)

# Specify forms for specific input edges
forms_in = (in_2 = ProjectedTo(MvNormalMeanCovariance, 2),)

# Create CVIProjection with custom forms
meta = DeltaMeta(
    method = CVIProjection(
        out_prjparams = form_out,
        in_prjparams = forms_in,
        ...
    )
)

Benefits

  • Flexible per-edge form specification
  • Maintains optimal performance where possible
  • Graceful handling of form mismatches
  • No breaking changes - all new parameters are optional

Let me know if any parts need clarification or expansion!

@Nimrais Nimrais marked this pull request as draft November 6, 2024 12:23
@bvdmitri
Copy link
Member

bvdmitri commented Nov 6, 2024

It seems to me that ProjectionForm(MvNormalMeanCovariance, (2,), nothing) basically copies the interface of ProjectedTo(MvNormalMeanCovariance, 2, conditioner = nothing). Can we maybe use the ProjectedTo instead of introducing a new structure? By doing that you can remove the create_project_to_ins and create_project_to entirely.

@Nimrais
Copy link
Member Author

Nimrais commented Nov 6, 2024

It seems to me that ProjectionForm(MvNormalMeanCovariance, (2,), nothing) basically copies the interface of ProjectedTo(MvNormalMeanCovariance, 2, conditioner = nothing). Can we maybe use the ProjectedTo instead of introducing a new structure? By doing that you can remove the create_project_to_ins and create_project_to entirely.

There are pros and cons to this suggestion.

The problem arises from our design decisions in ExponentialFamilyProjection.jl, where we've tightly coupled ProjectedTo with parameters. This coupling makes the interface less flexible than ideal. If we were to remove prjparams from CVIProjection and use ProjectedTo exclusively, we would be forced to specify the form even in cases where we only want to specify the parameters and allow the form to change dynamically during inference time.

By keeping ProjectionForm separate, we maintain a cleaner separation of concerns:

  • ProjectionForm handles just the form specification
  • The projection parameters can be handled independently through prjparams

Or do you mean to have a union for prjparams where it could be ExponentialFamilyProjection.ProjectionParameters or ProjectedTo.

@bvdmitri
Copy link
Member

bvdmitri commented Nov 6, 2024

This coupling makes the interface less flexible than ideal.

I don't agree with this, lets discuss in person. Having ability to pass parameters is even better.

we would be forced to specify the form even in cases where we only want to specify the parameters

Not really, you can always do ProjectedTo(ExponentialFamilyDistribution, parameters = ...) or simply dispatch on ExponentialFamilyProjection.ProjectionParameters. That's why I suggested you to simply reuse the prjparams field because its exactly what you want here. You duplicate structures that already exists and is supposed to solve the problem. Again lets better discuss in person.

@Nimrais Nimrais marked this pull request as ready for review November 12, 2024 14:50
ext/ReactiveMPProjectionExt/rules/marginals.jl Outdated Show resolved Hide resolved
ext/ReactiveMPProjectionExt/rules/marginals.jl Outdated Show resolved Hide resolved
ext/ReactiveMPProjectionExt/rules/out.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.93%. Comparing base (9af77aa) to head (adc2052).
Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   72.62%   71.93%   -0.70%     
==========================================
  Files         190      190              
  Lines        5454     5466      +12     
==========================================
- Hits         3961     3932      -29     
- Misses       1493     1534      +41     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Nimrais
Copy link
Member Author

Nimrais commented Nov 13, 2024

@bvdmitri codecoverage is working now! I see coverage for the project fails however it's good for the patch. And the project codecoverage report is not informative for me.

@bvdmitri
Copy link
Member

Yeah afaik project coverage in Julia is stochastic, so it can fluctuate a bit, no worries. I approved the PR, you can merge, good job!

@Nimrais Nimrais merged commit 05f0394 into main Nov 14, 2024
4 of 5 checks passed
@Nimrais Nimrais deleted the implement-additional-forms branch November 14, 2024 11:11
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.

2 participants