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

feat: added cubature recombination example. #798

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

tc85324
Copy link
Contributor

@tc85324 tc85324 commented Oct 2, 2024

Closes #778

PR Type

  • Feature
  • Tests

Description

Adds a test to demonstrate the end-to-end usage of TreeRecombination
for constructing non-product cubature formulae from product cubature
formulae. A corresponding integration test has also been added for this
new example.

How Has This Been Tested?

The new tests pass locally and contain theoretically grounded assertions.

Does this PR introduce a breaking 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 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.

Adds a test to demonstrate the end-to-end usage of `TreeRecombination`
for constructing non-product cubature formulae from product cubature
formulae. A corresponding integration test has also been added for this
new example.

refs: #778
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Performance review

Commit babd120 - Merge 199219b into 82ab5ec

No significant changes to performance.

@tc85324 tc85324 requested a review from tp832944 October 3, 2024 13:17
@tp832944 tp832944 self-assigned this Oct 4, 2024
Copy link
Contributor

@pc532627 pc532627 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 documentation comments - additionally the doc builds fail - this might be resolved simply by merging in main to this branch

examples/cubature_recombination.py Outdated Show resolved Hide resolved
examples/cubature_recombination.py Outdated Show resolved Hide resolved
examples/cubature_recombination.py Outdated Show resolved Hide resolved
@pc532627 pc532627 assigned pc532627 and unassigned tp832944 Oct 9, 2024
@pc532627
Copy link
Contributor

pc532627 commented Oct 9, 2024

@tc85324 Related to end-to-end usage of recombination, I'd be interested in your thoughts on how to sensibly include Recombination into the benchmarking currently being done on PR #802 . In that PR, @qh681248 is exploring how the various coreset methods compare in terms of run-time and error (MMD, KSD, ...) on a suite of candidate test problems. Since recombination isn't an ExplicitSizeSolver direct comparisons to coresets from other methods of a given size may not be in some sense fair. We could exclude it from benchmarking because of this - but if you have any ideas on sensible ways to include it we'd be open to suggestions.

Completes docstrings that were missed in a previous commit, simplifies
some logic to aid in code interpretability, and corrects some typehints.

Refs: #798
Copy link
Contributor

Performance review

Commit 7869814 - Merge 87bc9d8 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.6109 units ± 0.02807 units; execution 0.1594 units ± 0.001063 units
    • Significant increase in execution time (12.22%, p=1.457e-13)

Normalisation values for new data:
Compilation: 1 unit = 369.59 ms
Execution: 1 unit = 784.78 ms

Copy link
Contributor Author

@tc85324 tc85324 left a comment

Choose a reason for hiding this comment

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

@tc85324 Related to end-to-end usage of recombination, I'd be interested in your thoughts on how to sensibly include Recombination into the benchmarking currently being done on PR #802 . In that PR, @qh681248 is exploring how the various coreset methods compare in terms of run-time and error (MMD, KSD, ...) on a suite of candidate test problems. Since recombination isn't an ExplicitSizeSolver direct comparisons to coresets from other methods of a given size may not be in some sense fair. We could exclude it from benchmarking because of this - but if you have any ideas on sensible ways to include it we'd be open to suggestions.

Thank you for the review! Regarding benchmarking, I would say that the example in this pull is probably the canonical use case for recombination and it would be useful to have runtime performance figures for varying numbers of test-functions (dimension and degree), and in the case of TreeRecombination, tree_reduction_factor. While this won't be comparible to the ExplicitSizeSolvers it is useful from an implementaiton perspective as we can check/see if real world performance scales at the same/simlar rates to those claimed in the literature.

@tc85324 tc85324 requested a review from pc532627 October 18, 2024 06:28
@pc532627 pc532627 merged commit fd334fd into main Oct 18, 2024
23 checks passed
@pc532627 pc532627 deleted the feat/recombination-integration-test branch October 18, 2024 09:07
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.

Write integration test for tree recombination
3 participants