Create CoreSubset and KernelHerding classes #210

tp832944 · 2023-10-26T16:58:04Z

PR Type

Refactoring (no functional changes)
Documentation content changes

Description

Create Coreset ABC and KernelHerding classes as part of transitioning the codebase to OOP.

How Has This Been Tested?

Test A: TODO
Test B: (Write your answer here.)
Test C: (Write your answer here.)

Does this PR introduce a breaking change?

Yes, kernel_herding.py moved to coreset.py and switched from functional to OOP.

Checklist before requesting a review

I have made sure that my PR is not a duplicate.
My code follows the style guidelines of this project.
I have commented my code, particularly in hard-to-understand areas.
I have performed a self-review of my code.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.
Any dependent changes have been merged and published in downstream modules.

Refs #188

refs: #164

Methods not populated. refs: #164

Refs #188

…ge class Refs: #163

Refs: #163

Refs: #162

…create_data_reduction_abc

Refs: #164

pc532627 · 2023-12-21T16:59:01Z

@tp832944 @bk958178 - Updated to match (as reasonably as possible) the expected OOP interface. Needs merging into the OOP and updating inline with that.

pc532627 · 2023-12-22T08:41:04Z

Just a note that when we are confident this functions with the rest of the codebase, we need to delete the legacy .py scripts such as kernel_herding.py and kernel_herding_refine.py and any associated tests.

…te review

tp832944

@pc532627 I have merged oop_106, resolved conflicts, cleared out several files and reconciled changes so that I can see what this branch entails. If I have deleted one of your changes, please revert it.

I have reviewed coresubset.py with some detailed comments primarily to harmonise the interface. Functionally it looks fine. However, I haven't had a chance to review the tests yet.

coreax/coresubset.py

Refs: #164

tp832944 · 2024-01-02T14:58:47Z

@pc532627 I've been through my previous review and highlighted a couple of points that aren't quite right yet. I have yet to review the tests.

Refs: #164

pc532627 · 2024-01-02T15:13:11Z

@pc532627 I've been through my previous review and highlighted a couple of points that aren't quite right yet. I have yet to review the tests.

@tp832944 I've pushed some updates that hopefully address all of the outstanding comments.

tp832944 · 2024-01-02T15:31:09Z

@pc532627 I've been through my previous review and highlighted a couple of points that aren't quite right yet. I have yet to review the tests.

@tp832944 I've pushed some updates that hopefully address all of the outstanding comments.

@pc532627 All done. I'll get onto reviewing the tests later.

tp832944

@pc532627 I have now completed the full review, including the tests and functionality of kernel herding. Your tests are well documented. My remaining corrections are mostly stylistic.

tp832944 · 2024-01-02T18:18:53Z

coreax/coresubset.py

+        :returns: Updated loop variables ``current_coreset_indices`` and
+            ``current_kernel_similarity_penalty``
+        """
+        # Unpack the components of the updatable


updatable is not a noun. Change to loop variables.

tp832944 · 2024-01-02T18:26:46Z

coreax/coresubset.py

+        penalty_update = kernel_vectorised(
+            x, jnp.atleast_2d(x[index_to_include_in_coreset])
+        )[:, 0]
+        current_kernel_similarity_penalty = (


Use += operator. Easier to read. (Sorry in advance if this isn't valid syntax for Jax Arrays.)

After a quick check, I think this syntax is fine and have updated it.

tp832944 · 2024-01-02T18:39:19Z

coreax/coresubset.py

+            ``current_kernel_similarity_penalty``
+        """
+        # Unpack the components of the updatable
+        (current_coreset_indices, current_kernel_similarity_penalty) = val


Remove unnecessary brackets.

tp832944 · 2024-01-02T18:41:44Z

tests/unit/test_coresubset.py

+        self.coreset_size = 20
+
+    def test_fit_comparison_to_random_and_refined(self) -> None:
+        r"""


No need for the r on most of these docstrings.

tp832944 · 2024-01-02T18:42:07Z

tests/unit/test_coresubset.py

+        Test the fit method of the KernelHerding class with a simple example.
+
+        The test checks that a coreset generated via kernel herding has an improved
+        quality (measured my maximum mean discrepancy) that one generated by random


my -> by
that -> than

tp832944 · 2024-01-02T18:55:41Z

tests/unit/test_coresubset.py

+from coreax.coresubset import KernelHerding, RandomSample
+from coreax.metrics import MMD
+from coreax.reduction import SizeReduce
+from tests.unit.test_data import DataReaderConcrete


Delete this import. See related comment below for better alternative.

tp832944 · 2024-01-02T19:02:23Z

tests/performance/test_coresubset.py

+import coreax.coresubset as cs
+import coreax.kernel as ck
+from coreax.util import jit_test
+from tests.unit.test_data import DataReaderConcrete


Delete. See comment below for justification.

tp832944 · 2024-01-02T19:04:06Z

tests/performance/test_coresubset.py

+
+        # Create a kernel herding object
+        herding_object = cs.KernelHerding(
+            original_data=data,


Removed to align with new interface

tp832944 · 2024-01-02T19:04:13Z

tests/performance/test_coresubset.py

+            original_data=data,
+            weights_optimiser=None,
+            kernel=kernel,
+            coreset_size=self.coreset_size,


Removed to align with new interface

tp832944 · 2024-01-02T19:05:16Z

tests/performance/test_coresubset.py

+            )
+        )
+        kernel = ck.SquaredExponentialKernel()
+        data = DataReaderConcrete(original_data=x, pre_coreset_array=x)


I don't think data is required anymore after adjustments below to match updated interface. The method being tested effectively reads from x directly.

Correct - thanks for spotting, I've removed that and the now redundant import.

Refs: #164

bk958178

Suggesting a very minor change, @pc532627, but otherwise it looks good to me

bk958178 · 2024-01-03T11:09:30Z

tests/unit/test_coresubset.py

-            weights_optimiser=None,
-            kernel=kernel,
-        )
+        data_reduction_object_herding = KernelHerding(kernel=kernel)


Consider renaming to coresubset_object_herding or similar since DataReduction is outdated

bk958178 · 2024-01-03T11:10:33Z

tests/unit/test_coresubset.py

-        data_reduction_object_herding = KernelHerding(
-            weights_optimiser=None, kernel=kernel
-        )
+        data_reduction_object_herding = KernelHerding(kernel=kernel)


Consider renaming - see above comment

bk958178 · 2024-01-03T11:10:52Z

tests/unit/test_coresubset.py

-        data_reduction_object_herding = KernelHerding(
-            weights_optimiser=None, kernel=kernel
-        )
+        data_reduction_object_herding = KernelHerding(kernel=kernel)


Consider renaming - see above

Refs: #164

bk958178 and others added 13 commits October 16, 2023 11:32

feat: create refine classes

5c287e8

feat: update refine classes

019d222

feat: add abstract base class and method

576dad4

docs: update refine docstrings

2c22659

feat: kernel parameters in init of self

3f6813b

feat: update docstrings for refine classes

1fe3065

feat: make variable names clearer

e53c57a

feat: merge refine tests into branch

1b4ca0f

feat: update refine unit tests to OOP

d910dbc

Refs #188

feat: update jit statements

de5951f

Refs #188

feat: update unit tests

839a084

Refs #188

feat: update unit tests

2159525

Refs #188

feat: define Coreset ABC

bbfe3b9

refs: #164

tp832944 linked an issue Oct 26, 2023 that may be closed by this pull request

Create CoreSubset and KernelHerding classes #164

Closed

tp832944 self-assigned this Oct 26, 2023

tp832944 and others added 15 commits October 26, 2023 18:03

feat: define KernelHerding class

5ff9875

Methods not populated. refs: #164

feat: add pytrees for refine class

95800c0

Refs #188

feat: set up refine class factory

a7f9308

Refs #188

chore: merge util.py from oop_106

b7d871d

Refs #188

merge changes to kernel.py from branch feature/oop_106

2ceb1ff

feat: use Kernel class instead of KernelFunction from util.py

3e1c0be

Refs #188

refactor: remove kernel row mean from function inputs

e3de432

Refs #188

refactor: replace k_pairwise attribute with Kernel class method

08de3c4

Refs #188

chore: remove k_pairwise from doctring

96dfe0c

Refs #188

feat: initial structure of DataReader class and implementation of Ima…

6d02ddb

…ge class Refs: #163

feat: add class factories

c8fdb08

Refs: #163

merge changes to kernel.py from oop_stein_kernel_194

c860ee0

feat: rename of S variable and call kernel methods.

81d7acc

Refs: #162

feat: add DataReduction as input and merge reduction.py from feature/…

ba35bbf

…create_data_reduction_abc

feat: use DataReduction object

35f3a43

bk958178 added 2 commits December 13, 2023 11:40

feat: amend module names in coresubset.py and test_coresubset.py

76983ef

feat: update classes CoreSubset(Coreset) and RandomSample(Coreset)

b555739

bk958178 assigned bk958178 and unassigned tp832944 Dec 19, 2023

tp832944 changed the title ~~Create Coreset and KernelHerding classes~~ Create CoreSubset and KernelHerding classes Dec 20, 2023

bk958178 and others added 3 commits December 20, 2023 15:01

wip: KernelHerding class

a1550f3

feat: Add kernel herding and unit/performance tests.

5ec6d94

Refs: #164

feat: adapt herding to OOP interface.

f52e7c5

Refs: #164

pc532627 requested a review from bk958178 December 21, 2023 16:57

tp832944 added 3 commits December 22, 2023 16:41

Merge branch 'feature/oop_106' into feature/coreset_oop

df9ff3a

chore: remove code copied in from other branches in order to facilita…

abe6f8d

…te review

Reinstate refine.py from feature/oop_106

6c54e0e

tp832944 commented Dec 22, 2023

View reviewed changes

feat: update to align with OOP structure.

b11c23b

Refs: #164

pc532627 self-requested a review January 2, 2024 13:16

feat: style updates

9326aee

Refs: #164

pc532627 removed the request for review from bk958178 January 2, 2024 15:11

tp832944 commented Jan 2, 2024

View reviewed changes

feat: updates to tests to align with new interface.

3a7a3a4

Refs: #164

bk958178 requested changes Jan 3, 2024

View reviewed changes

pc532627 added 3 commits January 3, 2024 11:22

feat: variable naming updates.

763def3

Refs: #164

Merge branch 'feature/oop_106' into feature/coreset_oop

7380508

feat: removed unused KernelFunctionWithGrads definition

8073cd9

pc532627 merged commit 7af3297 into feature/oop_106 Jan 3, 2024
1 of 4 checks passed

pc532627 deleted the feature/coreset_oop branch January 3, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create CoreSubset and KernelHerding classes #210

Create CoreSubset and KernelHerding classes #210

tp832944 commented Oct 26, 2023 •

edited

Loading

pc532627 commented Dec 21, 2023

pc532627 commented Dec 22, 2023

tp832944 left a comment

tp832944 commented Jan 2, 2024

pc532627 commented Jan 2, 2024

tp832944 commented Jan 2, 2024

tp832944 left a comment

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

tp832944 Jan 2, 2024

pc532627 Jan 3, 2024

bk958178 left a comment

bk958178 Jan 3, 2024

pc532627 Jan 3, 2024

bk958178 Jan 3, 2024

pc532627 Jan 3, 2024

bk958178 Jan 3, 2024

pc532627 Jan 3, 2024

Create CoreSubset and KernelHerding classes #210

Create CoreSubset and KernelHerding classes #210

Conversation

tp832944 commented Oct 26, 2023 • edited Loading

PR Type

Description

How Has This Been Tested?

Does this PR introduce a breaking change?

Checklist before requesting a review

pc532627 commented Dec 21, 2023

pc532627 commented Dec 22, 2023

tp832944 left a comment

Choose a reason for hiding this comment

tp832944 commented Jan 2, 2024

pc532627 commented Jan 2, 2024

tp832944 commented Jan 2, 2024

tp832944 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bk958178 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tp832944 commented Oct 26, 2023 •

edited

Loading