Feature/benchmarking #802

qh681248 · 2024-10-09T09:18:52Z

PR Type

Feature

Description

Added two files mnist_benchmark.py and blobs_benchmark.py

How Has This Been Tested?

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

Does this PR introduce a breaking change?

(Write your answer here.)

Screenshots

(Write your answer here.)

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.

github-actions · 2024-10-09T09:20:52Z

Performance review

Commit `8a9d459` - Merge `1dffbc6` into `7aa60a7`

No significant changes to performance.

github-actions · 2024-10-10T09:43:02Z

Performance review

Commit `c281065` - Merge `055206b` into `1832579`

No significant changes to performance.

github-actions · 2024-10-10T11:28:12Z

Performance review

Commit `91933db` - Merge `78cd2e0` into `1832579`

No significant changes to performance.

github-actions · 2024-10-10T13:02:19Z

Performance review

Commit `4be645f` - Merge `9f799bf` into `1832579`

No significant changes to performance.

tp832944

I haven't got my head around everything yet, but this is good progress and you're clear to continue the job on benchmarking. I may have chucked on quite a few comments, although nothing is major.

tp832944 · 2024-10-10T13:39:07Z

CHANGELOG.md

Add benchmarking to change log.

benchmark/blobs_benchmark.py

benchmark/mnist_benchmark_visualiser.py

tp832944 · 2024-10-10T15:21:10Z

benchmark/blobs_benchmark.py

I don't think there are any sensible unit tests to write for this module.

tp832944 · 2024-10-10T15:27:54Z

benchmark/mnist_benchmark.py

+
+    """
+    data_loader = DataLoader(pytorch_data, batch_size=len(pytorch_data))
+    _data, _targets = next(iter(data_loader))  # Load all data at once


I'm not familiar with DataLoader. next(iter( would normally only grab the first item of something. Does this fetch all the data?

tp832944 · 2024-10-10T15:29:41Z

benchmark/mnist_benchmark.py

This is a pretty involved file. I would look at adding some unit tests for the medium-sized functions.

Seconding @tp832944 's comment - please add some unit tests

github-actions · 2024-10-11T16:37:33Z

Performance review

Commit `efb2040` - Merge `b84ff03` into `1832579`

No significant changes to performance.

github-actions · 2024-10-14T09:07:30Z

Performance review

Commit `1d62686` - Merge `41f4f09` into `1832579`

No significant changes to performance.

github-actions · 2024-10-17T09:46:47Z

Performance review

Commit `e1f9141` - Merge `5d0a8e2` into `3463539`

Statistically significant changes

basic_rpc:
- OLD: compilation 0.6253 units ± 0.03252 units; execution 0.142 units ± 0.001823 units
- NEW: compilation 0.6087 units ± 0.02183 units; execution 0.1634 units ± 0.00134 units
- Significant increase in execution time (15.05%, p=8.125e-16)

Normalisation values for new data:
Compilation: 1 unit = 375.27 ms
Execution: 1 unit = 795.81 ms

github-actions · 2024-10-17T10:46:14Z

Performance review

Commit `63937b4` - Merge `4180f52` into `3463539`

Statistically significant changes

basic_rpc:
- OLD: compilation 0.6253 units ± 0.03252 units; execution 0.142 units ± 0.001823 units
- NEW: compilation 0.6025 units ± 0.03407 units; execution 0.1573 units ± 0.0005494 units
- Significant increase in execution time (10.72%, p=7.818e-11)

Normalisation values for new data:
Compilation: 1 unit = 372.3 ms
Execution: 1 unit = 793.52 ms

…marking

…ation

github-actions · 2024-10-17T14:25:18Z

Performance review

Commit `65b253f` - Merge `db3d3ee` into `3463539`

No significant changes to performance.

…laining the benchmark process.

github-actions · 2024-10-17T14:41:44Z

Performance review

Commit `88742a6` - Merge `b912461` into `3463539`

Statistically significant changes

basic_rpc:
- OLD: compilation 0.6253 units ± 0.03252 units; execution 0.142 units ± 0.001823 units
- NEW: compilation 0.6208 units ± 0.02652 units; execution 0.1588 units ± 0.0007635 units
- Significant increase in execution time (11.78%, p=4.115e-12)

Normalisation values for new data:
Compilation: 1 unit = 367.39 ms
Execution: 1 unit = 792.56 ms

…marking

github-actions · 2024-10-18T09:02:51Z

Performance review

Commit `7746798` - Merge `e94aed0` into `ca26716`

Statistically significant changes

basic_rpc:
- OLD: compilation 0.6179 units ± 0.03292 units; execution 0.1748 units ± 0.0007481 units
- NEW: compilation 0.6272 units ± 0.03453 units; execution 0.155 units ± 0.001498 units
- Significant decrease in execution time (-11.34%, p=8.204e-15)
basic_stein:
- OLD: compilation 4.631 units ± 0.3963 units; execution 0.6794 units ± 0.01023 units
- NEW: compilation 4.589 units ± 0.3369 units; execution 0.5276 units ± 0.009367 units
- Significant decrease in execution time (-22.35%, p=8.009e-18)

Normalisation values for new data:
Compilation: 1 unit = 367.93 ms
Execution: 1 unit = 801.31 ms

bk958178 · 2024-10-23T10:56:46Z

Note: Upon fix of RPCholesky (once #787 and related fixes are Done), we expect benchmarking metrics to change significantly.

bk958178

@qh681248 Please find some further requested changes below.

This is an intermediate review. I'm having trouble running mnist_benchmark.py. Once I've resolved the issue, I will continue reviewing this PR

bk958178 · 2024-10-23T12:32:38Z

benchmark/blobs_benchmark.py

+
+def main() -> None:
+    """
+    Perform a benchmark comparing different coreset algorithms on a synthetic dataset.


Thanks for adding more information to this docstring, in response to a previous review comment.

It is still grammatically clumsy.
"Perform a benchmark..." is redundant, since 'benchmark' is a verb.
"...for each solver at different." isn't a correct ending to a sentence. Is the word 'sizes' missing?

Suggested rephrasing:

Benchmark different algorithms against a synthetic dataset. Compare the performance of different coreset algorithms using a synthetic dataset, generated using :func:`sklearn.datasets.make_blobs`. We set up various solvers, generate coresets of multiple sizes, and compute performance metrics (MMD and KSD) for each solver at each coreset size. Results are saved to a JSON file.

bk958178 · 2024-10-23T12:37:00Z

benchmark/blobs_benchmark.py

+
+The benchmarking process follows these steps:
+1. Generate a synthetic dataset of 1000 two-dimensional points using
+   `sklearn.datasets.make_blobs`.


Add :func: before sklearn reference, so the documentation externally links correctly.

Suggested change

`sklearn.datasets.make_blobs`.

:func:`sklearn.datasets.make_blobs`.

bk958178 · 2024-10-23T12:38:21Z

benchmark/blobs_benchmark.py

+
+import json
+import time
+from typing import Any, Tuple


Remove Tuple, and use type-annotations below with tuple instead (also requested in #802 (comment))

bk958178 · 2024-10-23T12:45:23Z

benchmark/blobs_benchmark.py

+        all_results[size] = results
+
+    # Save results to JSON file
+    with open("coreset_comparison_results.json", "w", encoding="utf-8") as f:


Why is this filename different from the one in blobs_benchmark_visualiser.py ?

Thanks for catching that, I have changed it to the correct file name now.

bk958178 · 2024-10-23T12:49:15Z

benchmark/blobs_benchmark.py

+def setup_stein_kernel(
+    sq_exp_kernel: SquaredExponentialKernel, dataset: Data
+) -> SteinKernel:
+    """Set up Stein Kernel."""


Document parameters and return for this function

:param sq_exp_kernel: ...
:param dataset: ...
:return: ...

bk958178 · 2024-10-23T12:54:29Z

benchmark/mnist_benchmark.py

+
+
+def save_results(results: dict) -> None:
+    """Save results to JSON."""


Document parameters!

bk958178 · 2024-10-23T13:17:57Z

benchmark/mnist_benchmark.py

+import jax.numpy as jnp
+import numpy as np
+import optax
+import torchvision


@qh681248 please add torch and torchvision. This file can't be run without these.

Please add them to pyproject.toml

bk958178 · 2024-10-23T13:35:43Z

.cspell/library_terms.txt

@@ -109,6 +109,7 @@ rcond
 rect
 rerunfailures
 rightarrow
+rngs


this is in custom_misc.txt too. It should only be needed in one of these .cspell files

bk958178 · 2024-10-23T13:36:20Z

benchmark/mnist_benchmark_visualiser.py

+
+
+def main() -> None:
+    """Load benchmark results and visualize the algorithm performance."""


Use British spelling: visualize -> visualise

bk958178 · 2024-10-23T13:53:31Z

benchmark/mnist_benchmark.py

Seconding @tp832944 's comment - please add some unit tests

bk958178

@qh681248 Some further questions/requested changes. Thanks in advance for responding

bk958178 · 2024-10-24T14:18:54Z

pyproject.toml

@@ -34,6 +34,8 @@ dependencies = [
    "scikit-learn",
    "tqdm",
    "typing-extensions",
+    "pytorch",


"pytorch" isn't the package named for PyTorch. It's "torch"

You can learn this by trying to run uv sync with this erroneous pyproject.toml file

bk958178 · 2024-10-25T08:17:36Z

.cspell/library_terms.txt

@@ -123,6 +124,7 @@ tensordot
 texttt
 toctree
 tomli
+torchvision


consider adding torch to this file

bk958178 · 2024-10-25T11:03:24Z

benchmark/mnist_benchmark_results.json

Is there a particular reason we're committing these results files? I would have assumed they didn't need to be added to the repo, as they can be recreated locally

This one takes a lot of runtime and computing power to generate, so we thought that it is worth committing (especially if we run it on a powerful computer to get the results)

bk958178 · 2024-10-25T11:03:49Z

benchmark/blobs_benchmark_results.json

Is there a particular reason we're committing these results files? I would have assumed they didn't need to be added to the repo, as they can be recreated locally

I don't think this one needs to be added as it can be recreated locally.

bk958178 · 2024-10-25T11:09:49Z

benchmark/blobs_benchmark.py

+
+    # Save results to JSON file
+    with open("blobs_benchmark_results.json", "w", encoding="utf-8") as f:


Add more control over where this results file is saved. Suggested change:

base_dir = os.path.dirname(os.path.abspath(__file__)) # Gets the directory of current script # Save results to JSON file with open(os.path.join(base_dir,"blobs_benchmark_results.json"), "w", encoding="utf-8") as f:

bk958178 · 2024-10-25T11:19:37Z

benchmark/mnist_benchmark_visualiser.py

+
+def main() -> None:
+    """Load benchmark results and visualise the algorithm performance."""
+    with open("mnist_benchmark_results.json", "r", encoding="utf-8") as file:


Similarly for blobs, enforce the location of the .json file. Suggested change:

base_dir = os.path.dirname(os.path.abspath(__file__)) # Gets the directory of current script # Save results to JSON file with open(os.path.join(base_dir,"mnist_benchmark_results.json"), "w", encoding="utf-8") as f:

…marking # Conflicts: # uv.lock

github-actions · 2024-10-25T14:51:38Z

Performance review

Commit `52dec04` - Merge `4b36684` into `458f15a`

Statistically significant changes

basic_herding:
- OLD: compilation 0.5307 units ± 0.07855 units; execution 0.1929 units ± 0.008614 units
- NEW: compilation 0.5451 units ± 0.09524 units; execution 0.2193 units ± 0.008304 units
- Significant increase in execution time (13.71%, p=1.605e-06)
basic_rpc:
- OLD: compilation 0.6136 units ± 0.02734 units; execution 0.1565 units ± 0.0008416 units
- NEW: compilation 0.6532 units ± 0.04273 units; execution 0.1849 units ± 0.005503 units
- Significant increase in execution time (18.15%, p=3.514e-08)
basic_stein:
- OLD: compilation 4.687 units ± 0.3033 units; execution 0.5352 units ± 0.01495 units
- NEW: compilation 4.573 units ± 0.434 units; execution 0.7048 units ± 0.02811 units
- Significant increase in execution time (31.71%, p=1.482e-10)

Normalisation values for new data:
Compilation: 1 unit = 383.73 ms
Execution: 1 unit = 811.5 ms

github-actions · 2024-10-25T15:22:57Z

Performance review

Commit `960bec0` - Merge `f679a57` into `458f15a`

No significant changes to performance.

github-actions · 2024-10-25T16:03:24Z

Performance review

Commit `5106664` - Merge `a2f45d3` into `458f15a`

No significant changes to performance.

…marking

github-actions · 2024-10-30T10:27:33Z

Performance review

Commit `9c17c98` - Merge `058e816` into `cf9f6c0`

No significant changes to performance.

Initial commit for benchmarking

1dffbc6

pc532627 mentioned this pull request Oct 9, 2024

feat: added cubature recombination example. #798

Merged

9 tasks

qh681248 added 2 commits October 10, 2024 10:40

Replaced small_dataset with train_data_jax

54f0daa

Added copyright and documentation

055206b

qh681248 linked an issue Oct 10, 2024 that may be closed by this pull request

Coreset algorithm benchmarks for different data types #509

Open

added docstring to __init__.py

78cd2e0

Added documentation on benchmark/__init__.py

9f799bf

Added main function on mnist_benchmark.py

5269d1c

tp832944 requested changes Oct 10, 2024

View reviewed changes

Added main() function on mnist_benchmark.py

b84ff03

qh681248 added 2 commits October 14, 2024 09:26

Added linestyle to custom_misc.txt

650fcdc

Wrapped mnist_benchmark_visualiser.py on the main function

41f4f09

feat(benchmark): add main function to all new benchmark scripts

5d0a8e2

feat(benchmark): add main function to all new benchmark scripts

4180f52

qh681248 added 4 commits October 17, 2024 14:22

Merge remote-tracking branch 'remotes/origin/main' into feature/bench…

9e7891a

…marking

feat: add blobs_benchmark_visualiser.py for benchmark result visualis…

e6baad2

…ation

feat: Add benchmark/blobs_benchmark_visualiser.py

52e7869

feat: renamed json files

db3d3ee

feat: Add docstrings on blobs_benchmark.py and mnist_benchmark.py exp…

b912461

…laining the benchmark process.

Merge remote-tracking branch 'remotes/origin/main' into feature/bench…

e94aed0

…marking

qh681248 marked this pull request as ready for review October 18, 2024 09:00

bk958178 self-requested a review October 21, 2024 08:40

bk958178 requested changes Oct 23, 2024

View reviewed changes

qh681248 added 3 commits October 23, 2024 16:00

:build: add pytorch, torchvision to dependencies

1df2237

:docs: fixed docstrings

09b6077

chore: Updated uv.lock after adding additional dependencies.

4cef166

bk958178 requested changes Oct 25, 2024

View reviewed changes

qh681248 added 4 commits October 25, 2024 12:36

feat: Added unit/test_benchmark.py

a86b09f

fix: Fix merge conflict due to uv.lock

4860981

fix: Fix merge conflict due to uv.lock

4196727

Merge remote-tracking branch 'remotes/origin/main' into feature/bench…

4b36684

…marking # Conflicts: # uv.lock

ci: remove macos-13 test from continuous integration test

f679a57

qh681248 added 4 commits October 25, 2024 16:56

refactor: use base directory for json path

00f069e

refactor: use base directory for json path

6102918

refactor: use base directory for json path

606c2b9

refactor: use base directory for json path

a2f45d3

qh681248 added 4 commits October 30, 2024 09:51

chore: Added torch to library_terms.txt

28fe378

Merge remote-tracking branch 'remotes/origin/main' into feature/bench…

880711e

…marking

chore: merge main to branch to resolve merge conflict

0da26bd

feat: increase block size to 128 to run on more powerful machine

058e816

	`sklearn.datasets.make_blobs`.
	:func:`sklearn.datasets.make_blobs`.



		def save_results(results: dict) -> None:
		"""Save results to JSON."""



		def main() -> None:
		"""Load benchmark results and visualize the algorithm performance."""


		# Save results to JSON file
		with open("blobs_benchmark_results.json", "w", encoding="utf-8") as f:

Feature/benchmarking #802

Are you sure you want to change the base?

Feature/benchmarking #802

Conversation

qh681248 commented Oct 9, 2024 • edited Loading

PR Type

Description

How Has This Been Tested?

Does this PR introduce a breaking change?

Screenshots

Checklist before requesting a review

github-actions bot commented Oct 9, 2024

Performance review

Commit 8a9d459 - Merge 1dffbc6 into 7aa60a7

github-actions bot commented Oct 10, 2024

Performance review

Commit c281065 - Merge 055206b into 1832579

github-actions bot commented Oct 10, 2024

Performance review

Commit 91933db - Merge 78cd2e0 into 1832579

github-actions bot commented Oct 10, 2024

Performance review

Commit 4be645f - Merge 9f799bf into 1832579

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

github-actions bot commented Oct 11, 2024

Performance review

Commit efb2040 - Merge b84ff03 into 1832579

github-actions bot commented Oct 14, 2024

Performance review

Commit 1d62686 - Merge 41f4f09 into 1832579

github-actions bot commented Oct 17, 2024

Performance review

Commit e1f9141 - Merge 5d0a8e2 into 3463539

Statistically significant changes

github-actions bot commented Oct 17, 2024

Performance review

Commit 63937b4 - Merge 4180f52 into 3463539

Statistically significant changes

github-actions bot commented Oct 17, 2024

Performance review

Commit 65b253f - Merge db3d3ee into 3463539

github-actions bot commented Oct 17, 2024

Performance review

Commit 88742a6 - Merge b912461 into 3463539

Statistically significant changes

github-actions bot commented Oct 18, 2024

Performance review

Commit 7746798 - Merge e94aed0 into ca26716

Statistically significant changes

bk958178 commented Oct 23, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qh681248 commented Oct 9, 2024 •

edited

Loading

Commit `8a9d459` - Merge `1dffbc6` into `7aa60a7`

Commit `c281065` - Merge `055206b` into `1832579`

Commit `91933db` - Merge `78cd2e0` into `1832579`

Commit `4be645f` - Merge `9f799bf` into `1832579`

Commit `efb2040` - Merge `b84ff03` into `1832579`

Commit `1d62686` - Merge `41f4f09` into `1832579`

Commit `e1f9141` - Merge `5d0a8e2` into `3463539`

Commit `63937b4` - Merge `4180f52` into `3463539`

Commit `65b253f` - Merge `db3d3ee` into `3463539`

Commit `88742a6` - Merge `b912461` into `3463539`

Commit `7746798` - Merge `e94aed0` into `ca26716`

bk958178 commented Oct 23, 2024 •

edited

Loading

Commit `52dec04` - Merge `4b36684` into `458f15a`

Commit `960bec0` - Merge `f679a57` into `458f15a`

Commit `5106664` - Merge `a2f45d3` into `458f15a`

Commit `9c17c98` - Merge `058e816` into `cf9f6c0`