gchq / coreax

A library for coreset algorithms, written in Jax for fast execution and GPU support.
Apache License 2.0
15 stars 1 forks source link

refactor: moved inversion functionality to inverses.py #649

Closed db091756 closed 2 weeks ago

db091756 commented 3 weeks ago

PR Type

Description

Adds helper functions to RegularisedInverseApproximator to map approximations across first axis. Moves invert_regularised_array from util.py from inverses.py as LeastSquareApproximator.

Increased default oversampling_parameter after more testing with CMMD solver.

How Has This Been Tested?

Moved invert_regularised_array tests from test_util.py to test_inverses.py . Added additional approximation map tests.

Does this PR introduce a breaking change?

(Write your answer here.)

Screenshots

(Write your answer here.)

Checklist before requesting a review

db091756 commented 3 weeks ago

My primary concerns here are that the approximate.py module is for approximating (overloading) the methods of coreax.kernel.Kernels. My secondary concern is that the inversion methods are (I think) unnecessarily (conceptually) coupled to the existance of a kernel or one of its matrices (E.G. the Gramian). Can these methods be generalised to the inversion of arbitraray matrices (subject to certain conditions)? If not, then I think these should be called BlockInverseApproximators or something similar, and moved into their own module inverses.py.

If the kernel coupling must exist, then maybe we want to add a compute_inverse method to kernel and have these approximators overload it? This way we are still defining ApproximateKernels, and we can compose multiple approximations.

The functions do work independently of the kernels, I would be happy to uncouple them and move them to their own file. Will do tomorrow

Done this now!

db091756 commented 3 weeks ago

Appologies for the dealy in getting back to this.

I'd like a few clarifications on mathematical terminology, mostly to check my own understading. Also, am I right to think all these methods be generalized to the solution of general (non-)linear systems?

Note there are also some pre-commit failures (pylint no-member)

No worries and thanks for the review!

We are either solving regularised least-square problems or approximating the solutions to them so far.

The pre-commit failures come from parts of the codebase that this pull doesn't touch and aren't being flagged locally so not sure what is happening. They stay when I re-run it also.