Skip to content

Conversation

@chrischu12
Copy link

No description provided.

Copy link
Owner

@simon-hirsch simon-hirsch left a comment

Choose a reason for hiding this comment

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

I miss the base class for the copulas, because now all distributions need to implement the param_link parameter. This should be solved by a CopulaMixin class that implements the necessary methods.

@simon-hirsch simon-hirsch requested review from BerriJ and Copilot July 3, 2025 09:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for bivariate copulas by introducing new link functions and multivariate copula distributions, and integrates them into the online multivariate distribution regression estimator.

  • Introduce FisherZLink and TauToPar in copulalinks.py
  • Extend base Distribution to accept param_links and update existing distributions
  • Add BiCopNormal and MarginalCopula classes for Gaussian copula modeling
  • Integrate copula-based parameter handling into online_mvdistreg

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/ondil/link/copulalinks.py New Fisher Z and Kendall‐tau link functions
src/ondil/link/init.py Export copula link functions
src/ondil/base/distribution.py Add support for param_links in base Distribution
src/ondil/distributions/normal.py Update constructor signature for param_links
src/ondil/distributions/mv_t_low_rank.py Pass param_links through to superclass
src/ondil/distributions/mv_normal_chol.py Pass param_links through to superclass
src/ondil/distributions/mv_marg_cop.py New MarginalCopula distribution
src/ondil/distributions/bicop_normal.py New BiCopNormal Gaussian copula distribution
src/ondil/distributions/init.py Register new copula distributions
src/ondil/estimators/online_mvdistreg.py Integrate copula option and branching logic into fitting routine
Comments suppressed due to low confidence (3)

src/ondil/distributions/normal.py:47

  • The param_links parameter should be typed as dict[int, LinkFunction] instead of LinkFunction, and default to an empty dict. Update the annotation to param_links: dict[int, LinkFunction] = {}.
        param_links: LinkFunction = {},

src/ondil/distributions/bicop_normal.py:33

  • The constructor uses a singular param_link but the base class expects param_links: dict[int, LinkFunction]. Consider renaming to param_links and accepting a mapping keyed by parameter index.
        param_link: LinkFunction = TauToPar(),

src/ondil/estimators/online_mvdistreg.py:690

  • The code previously scaled X and stored it in X_scaled but now passes the raw X to _outer_fit, bypassing the scaling step. It should likely be X_scaled here.
        self._outer_fit(X=X, y=y, theta=theta)

@simon-hirsch simon-hirsch mentioned this pull request Jul 3, 2025
@simon-hirsch simon-hirsch changed the base branch from mvdistreg to main September 11, 2025 14:09
@simon-hirsch
Copy link
Owner

@chrischu12 I have changed the base branch as we #134 is merged now.

@simon-hirsch simon-hirsch requested a review from Copilot October 8, 2025 07:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@BerriJ
Copy link
Collaborator

BerriJ commented Nov 6, 2025

What's the status on the tests in this PR? A lot of them fail.

And what about the numerous comments from @simon-hirsch above?
@chrischu12 Please mark them as resolved if they are not relevant anymore, to keep this PR overseeable.

@BerriJ
Copy link
Collaborator

BerriJ commented Nov 13, 2025

Now the Lint and Commit Job passes :)
Still, we need to fix the failing tests.

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.

3 participants