CUQI-DTU / CUQIpy

https://cuqi-dtu.github.io/CUQIpy/
Apache License 2.0
48 stars 9 forks source link

Regularized gaussian extension #424

Open jeverink opened 6 months ago

jeverink commented 6 months ago

Summary of added features:

Includes a notebook with various examples.

jeverink commented 6 months ago

@nabriis , If you have time, could you check the way I handled the conditioning related methods get_mutable_variables and _condition in RegularizedGaussian and RegularizedUniform to handle conditioning on both the underlying Gaussian parameters and the local strength parameter. Whilst the current implementation seems to work, I am uncertain about why it works, so it could possibly use some improvement.

jakobsj commented 6 months ago

Thank you very much @jeverink for this comprehensive set of new functionality!

This morning @nabriis, @chaozg and I discussed how to best progress this. From experience it often takes a rather long time to review and iterate on big PRs, and at the same time we have some related functionality on Gibbs and conjugates on the way in #425. Some of the additions should be more straightforward than others.

It would be very helpful and make the way into CUQIpy a lot quicker if you can split this into multiple separate PRs, each on separate new functionality. Following your split into bullet points, my suggestion would be to start with the smaller ones by adding a PR on the modified half gaussian, a PR on the regularized uniform, a PR on the Constrained and Nonnegative versions, one on ADMM, one on TV (if needed together with the ADMM one). Then save the Gibbs and any conjugacy stuff till the end, as that may be affected by the updates in #425.

nabriis commented 6 months ago

@nabriis , If you have time, could you check the way I handled the conditioning related methods get_mutable_variables and _condition in RegularizedGaussian and RegularizedUniform to handle conditioning on both the underlying Gaussian parameters and the local strength parameter. Whilst the current implementation seems to work, I am uncertain about why it works, so it could possibly use some improvement.

Hi @jeverink, the approach seems sensible! It might make sense to include any potential conditional variables and mutable variables from the underlying Gaussian in some cases. Once, as suggested by Jakob, we split this PR into smaller parts I can comment on the changes or suggest updates. It looks like it is not far from where it should be though.

jeverink commented 6 months ago

Hi @jakobsj @nabriis @chaozg , thanks for the suggestion.

I have now split (hopefully) all of the not conditioning/Gibbs/conjugate related features into 3 branches/PRs #429 #430 #431 .

430 contains the aliases RegulerizedUniform, Constrained/Nonnegative Gaussian/GMRF, so should be relatively straightforward.

429 contains the ModifiedHalfNormal distribution, needed for the conjugacy later.

431 contains ADMM together with the regularization/constraint options rework and Total Variation, so contains the most changes.

I hope this helps with the reviewing and iterating.

nabriis commented 6 months ago

Thanks @jeverink. Looks good. I will have a quick look now.