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.

Sign up for GitHub

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

Jump to bottom

Feature/benchmarking new solvers #971

Merged
merged 11 commits into from
Feb 24, 2025
Merged

Feature/benchmarking new solvers #971

merged 11 commits into from
Feb 24, 2025

Conversation

qh681248
Copy link
Contributor

@qh681248 qh681248 commented Feb 12, 2025 •
edited
Loading

PR Type

  • Feature
  • Documentation content changes

Description

Adds a new solver class Iterative Herding
Adds Compress++, and iterative herding to benchmarking scripts
Updates benchmarking results in documentation

How Has This Been Tested?

Existing tests pass as expected.

New tests introduced with this change verify that Iterative Herding method works as expected

Does this PR introduce a breaking change?

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.

@qh681248 qh681248 linked an issue Feb 12, 2025 that may be closed by this pull request
Add Compress++ and probabilistic KH to benchmarking #947
Closed
@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 351573b - Merge 4d16a47 into cc4d311

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit a5cd0bf - Merge 95a52fd into cc4d311

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 6286747 - Merge ee66b25 into cc4d311

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 500e10e - Merge 11a1b74 into cc4d311

Statistically significant changes

  • basic_stein:
    • OLD: compilation 5.819 units ± 0.3247 units; execution 0.5649 units ± 0.01893 units
    • NEW: compilation 5.708 units ± 0.1786 units; execution 0.6215 units ± 0.03212 units
    • Significant increase in execution time (10.01%, p=0.000254)

Normalisation values for new data:
Compilation: 1 unit = 356.49 ms
Execution: 1 unit = 802.56 ms

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit de7e15a - Merge f9ca991 into 8cfa213

No significant changes to performance.

@qh681248 qh681248 force-pushed the feature/benchmarking-new-solvers branch from f9ca991 to ce685db Compare February 14, 2025 12:51
@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 138e27f - Merge ce685db into 51a8d55

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 796af07 - Merge eb54f51 into 51a8d55

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit f5ef6ca - Merge 9f7b063 into 51a8d55

No significant changes to performance.

@rg936672 rg936672 self-requested a review February 17, 2025 13:01
rg936672
rg936672 requested changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@rg936672 rg936672 left a comment

Choose a reason for hiding this comment

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

Quite a few comments, but looks good!

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit b5d9848 - Merge 8246ec3 into 69add99

No significant changes to performance.

@qh681248 qh681248 requested a review from rg936672 February 20, 2025 15:22
@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit f5c5ef9 - Merge 9d4f15b into 69add99

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 9662c2a - Merge e2e3c9b into 69add99

No significant changes to performance.

rg936672
rg936672 requested changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@rg936672 rg936672 left a comment

Choose a reason for hiding this comment

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

A few more changes, plus some unresolved comments from the previous review.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 33b8b4f - Merge 3425c1a into cc77d8e

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 30c5ee8 - Merge dc37b6a into cc77d8e

No significant changes to performance.

@qh681248 qh681248 requested a review from rg936672 February 21, 2025 13:56
@rg936672 rg936672 merged commit bf1c62d into main Feb 24, 2025
23 checks passed
@rg936672 rg936672 deleted the feature/benchmarking-new-solvers branch February 24, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tp832944 tp832944 tp832944 left review comments

@rg936672 rg936672 rg936672 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Compress++ and probabilistic KH to benchmarking
3 participants
@qh681248 @tp832944 @rg936672