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/probabilistic kh example #953

Merged
merged 14 commits into from
Feb 10, 2025
Merged

Feature/probabilistic kh example #953

merged 14 commits into from
Feb 10, 2025

Conversation

gw265981
Copy link
Contributor

@gw265981 gw265981 commented Feb 6, 2025 •
edited
Loading

PR Type

  • Feature

Description

Added an example that runs probabilistic Kernel Herding and plots the results.

How Has This Been Tested?

Existing tests pass 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.

@gw265981
fix: remove temperature validation for probabilistic KH
d75636c 
The validator prevents the class being instantiated inside a JIT loop. jaxtyping.Scalar is also added for this purpose.
@gw265981
feat: add example for iterative probabilistic KH
7bf3c39
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Performance review

Commit 716bd34 - Merge 7bf3c39 into 16d8e62

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Performance review

Commit 86756b7 - Merge d2fb5a6 into 16d8e62

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Performance review

Commit 9471bd0 - Merge 002d782 into 16d8e62

No significant changes to performance.

@gw265981
Copy link
Contributor Author

gw265981 commented Feb 7, 2025

The coverage check is failing (with a 0.01% reduction), which I believe is false positive. The only tested file that is affected by this PR is coreax/solvers/coresubset.py and the only uncovered lines there are not affected by this PR.

Should be fine to merge in my opinion.

@gw265981 gw265981 marked this pull request as ready for review February 7, 2025 09:28
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 7, 2025

Performance review

Commit 11d5388 - Merge b6184e0 into 94a1a2d

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 7, 2025

Performance review

Commit e66b336 - Merge 0277c72 into 94a1a2d

No significant changes to performance.

@rg936672 rg936672 linked an issue Feb 7, 2025 that may be closed by this pull request
Example script for Probabilistic KH #946
Closed
@rg936672 rg936672 self-requested a review February 7, 2025 15:33
rg936672
rg936672 requested changes Feb 7, 2025
View reviewed changes
coreax/solvers/coresubset.py Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 0c5b0ab - Merge 62b3596 into 9bae5f7

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 331c098 - Merge b51907d into 9bae5f7

No significant changes to performance.

rg936672
rg936672 requested changes Feb 10, 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.

Looking good! A few more changes, plus one comment left unresolved from the last review.

examples/iterative_probabilistic_kh.py Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
examples/iterative_probabilistic_kh.py Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit a15ce79 - Merge 7a39b6d into 6715f82

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 59e1b17 - Merge 74d6115 into 6715f82

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 34ab9c0 - Merge df0c771 into 6715f82

No significant changes to performance.

@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 4867d29 - Merge d59b72c into bb20c65

No significant changes to performance.

@rg936672
Copy link
Contributor

Ignoring coverage pipeline failure - 0.01% reduction is essentially a false positive here.

@rg936672 rg936672 closed this Feb 10, 2025
@rg936672 rg936672 reopened this Feb 10, 2025
@rg936672 rg936672 merged commit 66177d9 into main Feb 10, 2025
36 of 37 checks passed
@rg936672 rg936672 deleted the feature/probabilistic_kh_example branch February 10, 2025 15:09
@github-actions GitHub Actions
Copy link
Contributor

Performance review

Commit 196cb94 - Merge d59b72c into d16bb65

No significant changes to performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

Example script for Probabilistic KH
2 participants
@gw265981 @rg936672