Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/add probabilistic iterative methods #983

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

gw265981
Copy link
Contributor

@gw265981 gw265981 commented Feb 19, 2025

PR Type

  • Feature

Description

Adds the reduce_iterative() method to KernelHerding. Closes #977.

How Has This Been Tested?

Existing tests pass as expected.

New tests introduced with this change verify that reduce_iterative() works correctly. Due to the probabilistic nature of the solver when probabilistic=True, the exact behaviour is hard to verify, hence the test replaces jax.random.choice with a deterministic choice function.

Does this PR introduce a breaking change?

No.

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 ensured my code is easy to understand, including docstrings and comments where necessary.
  • I have performed a self-review of my code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.
  • I have updated CHANGELOG.md, if appropriate.

Copy link
Contributor

Performance review

Commit da5d8bb - Merge 43385ae into 18ef363

No significant changes to performance.

Copy link
Contributor

Performance review

Commit b720fcd - Merge 57a425f into 18ef363

No significant changes to performance.

@gw265981 gw265981 marked this pull request as ready for review February 19, 2025 18:05
@gw265981 gw265981 marked this pull request as draft February 19, 2025 18:07
Copy link
Contributor

Performance review

Commit 941bc45 - Merge 538071a into d6f481e

No significant changes to performance.

Copy link
Contributor

Performance review

Commit 98892be - Merge 25a7afc into d6f481e

No significant changes to performance.

Copy link
Contributor

Performance review

Commit 8ff2d80 - Merge 9d61a7f into d6f481e

No significant changes to performance.

Copy link
Contributor

Performance review

Commit e17a760 - Merge d044c92 into d6f481e

No significant changes to performance.

@gw265981 gw265981 marked this pull request as ready for review February 27, 2025 09:42
@qh681248 qh681248 self-requested a review February 27, 2025 11:02
qh681248
qh681248 previously approved these changes Feb 27, 2025
Copy link
Contributor

@qh681248 qh681248 left a comment

Choose a reason for hiding this comment

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

Happy with the changes

@qh681248 qh681248 self-requested a review February 27, 2025 12:27
Copy link
Contributor

@qh681248 qh681248 left a comment

Choose a reason for hiding this comment

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

It might be worth adding it to changelog.md?
Feel free to pushback

Should the example script use this new method?

@rg936672 rg936672 linked an issue Feb 28, 2025 that may be closed by this pull request
Copy link
Contributor

Performance review

Commit ae38ac5 - Merge 17308db into 18e7c10

No significant changes to performance.

Copy link
Contributor

github-actions bot commented Mar 3, 2025

Performance review

Commit 2f1e2fd - Merge 1c96744 into e8d122c

Statistically significant changes

  • basic_herding:
    • OLD: compilation 0.7142 units ± 0.1192 units; execution 0.218 units ± 0.006644 units
    • NEW: compilation 0.7525 units ± 0.09401 units; execution 0.1953 units ± 0.01202 units
    • Significant decrease in execution time (-10.39%, p=0.00013)
  • basic_stein:
    • OLD: compilation 5.82 units ± 0.3454 units; execution 0.7235 units ± 0.02806 units
    • NEW: compilation 6.091 units ± 0.3224 units; execution 0.5361 units ± 0.01032 units
    • Significant decrease in execution time (-25.91%, p=3.463e-10)

Normalisation values for new data:
Compilation: 1 unit = 333.37 ms
Execution: 1 unit = 806.91 ms

Copy link
Contributor

github-actions bot commented Mar 3, 2025

Performance review

Commit 97681b3 - Merge 5a5c01f into e8d122c

Statistically significant changes

  • basic_stein:
    • OLD: compilation 5.82 units ± 0.3454 units; execution 0.7235 units ± 0.02806 units
    • NEW: compilation 5.618 units ± 0.342 units; execution 0.5579 units ± 0.01128 units
    • Significant decrease in execution time (-22.89%, p=9.072e-10)

Normalisation values for new data:
Compilation: 1 unit = 358.14 ms
Execution: 1 unit = 798.23 ms

@gw265981 gw265981 requested a review from pc532627 March 3, 2025 15:49
@gw265981 gw265981 marked this pull request as draft March 4, 2025 09:25
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.

Add probabilistic iterative methods Make Iterative KH padding invariant
2 participants