Feature/tree recombination #504
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tc85324
added
enhancement
tc85324
force-pushed
the
feature/tree-recombination
branch
from
ad63ef9 to
752cae6
Compare
tc85324
force-pushed
the
feature/tree-recombination
branch
from
752cae6 to
442e88e
Compare
tc85324
force-pushed
the
feature/tree-recombination
branch
2 times, most recently
from
c8685dc to
77072da
Compare
9 tasks
Additionally enhances coreset testing.
Adds a new `recombination.py` module to the solvers subpackage, providing a generalised `RecombinationSolver`, in addition to the `CaratheodoryRecombination` and `TreeRecombination` solvers. A `RecombinationSolverTest` case has been added to `test_solvers.py`, covering the known degenerate cases that may arise in recombination. The documentation has been updated where required, but no dedicated new examples/documentation pages have been created. Refs: #497
tc85324
force-pushed
the
feature/tree-recombination
branch
from
77072da to
fbe7f2b
Compare
|
Will try review this next week! Just merged in a refactor of |
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
reviewed
Aug 19, 2024
db091756
requested changes
Aug 19, 2024
|
Apologies this took a while to get to, trying to wrap up work before leaving has taken more time than expected |
Performed in response to reviewer comments on #504
Adds references to the original articles for Tchakaloff's theorem and Caratheodory's convex hull theorem (in French/German respectively), in addition to more recent, but for our purposes equivalent, references written in English.
Makes clearer the correspondence between the Caratheodory recombination algorithm implemented here, and the thesis of Tchernychova. Provides additional clarity on the steps performed in Tree recombination.
db091756
reviewed
Sep 3, 2024
db091756
reviewed
Sep 3, 2024
There was a problem hiding this comment.
I am much much happier with my understanding of the code/maths with these additional comments, thanks for adding!
Remaining is some hopefully simple Pyright fixes, and I think some of these private functions could live in util.py, e.g. _resolve_rcond, _prepare_tree, _centroid.
Also, what is your opinion on the private functions being added as static methods of the CaratheodoryRecombination class. Are there any JAX implications?
db091756
requested changes
Sep 3, 2024
Corrects some simple to fix pyright linting errors.
In attempting to placate the pyright linter a new, erroneous change was made to the logic in `_resolve_null_basis`. This has been fixed and the type hints updated appropriately.
Apparent runtime incompatibility with jaxtyping. Appears that the `Shaped[TypeVar, ...]` syntax used for `_ElementOfOmega is not supported (even if the TypeVar is bound by Array).
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Closes #497
Description
Provides
CaratheodoryRecombinationandTreeRecombinationsolvers that are JIT compatible and avoid shape based recursion (instead using the jax for loop and while loop primitives to minimise the compilation time).While every effort has been made to be as computationally/memory efficient as possible, I suspect there remain many ways to enhance the performance/efficiency of the implementations. However, for now, I believe the performance level is sufficient for the solvers to be useful (we should seek further performance as and when user requirements demand it).
This pull also makes a small tweak to the
Coresubsetmaterialisation and testing, as these changes aided in checking the correctness of the recombination implementations.How Has This Been Tested?
Dedicated tests have been added, which check for most (maybe all) degenerate cases that can arise during recombination. To minimise the additional time costs incurred by testing recombination, no additional recombination specific integration tests/examples have been added.
Does this PR introduce a breaking change?
No
Checklist before requesting a review