Map reduce index bug #790
Map reduce index bug #790
Conversation
… _jit_tree function to return indices as well, changed the reduce method to keep track of indices, added a line plt.show() to pounce.py
… _jit_tree function to return indices as well, changed the reduce method to keep track of indices, added a line plt.show() to pounce.py
…e method returns coreset with indices bigger than the leafsize as was present in #779. This test fails when that bug is present and passes when it is not present
… calculated index if base solver returns a Coresubset object and not just a Coreset object.
|
|
…y when _indices is not none. _jit_tree now returns jnp.arange(len(dataset))[indices] instead of indices so that it is within bounds.
There was a problem hiding this comment.
Thanks for spotting this bug, I should have caught this earlier! I think the issue is more fundamental to how Coresubset.coreset operates, and the error can be fixed by the following change (subject to correcting the coreset materialization behaviour).
# old
_coreset = jtu.tree_map(jnp.concatenate, coreset_ensemble)
return _reduce_coreset(_coreset.coreset)
# proposed
_coreset = jtu.tree_map(jnp.concatenate, coreset_ensemble.coreset)
return _reduce_coreset(_coreset)I'd like to have a look today to see if there is a nice change that can be made to the Coresubset.coreset materialization behaviour, before the specific changes proposed here are considered.
There was a problem hiding this comment.
Having taken a look yesterday, I can't find a particularly satisfying solution to the materialization behaviour. We could add specific logic for handling "batched" coresets in the coresubset materialization method Coresubset.coreset, but this would require us to handle a variety of different behaviours. E.G. what happens when we have unbatched nodes/indices, but a batched pre_coreset_data, what happens when the nodes/indices are batched but pre_coreset_data is not (this is the specific problem in our case), etc... If there is wider interest in properly handling batched data, then this may be worth revisiting in a separate issue, perhaps at the same time as #765.
In any case, I'm pleased you've identified the problem (which also indicates the existing MapReduce tests are insufficient) and proposed a solution in this PR, which I will be glad to review today.
|
@qh681248 This is a significant bug fix. Don't forget to add it to the CHANGELOG. |
…dex] to padded_dataset[index] removed unnessary comments and changed variable names to be more descriptive
@tc85324 if we haven’t heard from you by Thursday, we’ll assume you’re okay with not reviewing these changes before the v0.3 release. Any changes you request later will be addressed and included in future releases. |
Performance reviewCommit
|
There was a problem hiding this comment.
Thanks for adding this analytic test, @qh681248. Your description is very thorough. Please see some initial comments and suggested changes below.
I'll allow time for @tc85324 to review the analytic test as well, and share any thoughts or comments too.
@tc85324 if we haven’t heard from you by Thursday, we’ll assume you’re okay with not reviewing these changes before the v0.3 release. Any changes you request later will be addressed and included in future releases.
Unfortunately I don't have the time right now to review this properly. I did have a quick look though and the new tests appear to do what I was hoping for, thanks @qh681248!
bk958178
dismissed
tc85324’s stale review
Dismissing as re-reviewed and happy with changes
PR Type
Description
In coreax/solvers/composite.py, added logic to keep track of indices in the reduce method of MapReduce class, this resolves the Fix MapReduce solver Indexing bug #779
How Has This Been Tested?
Added test_mapreduce_diverse_selection in tests/units/test_solvers.py. It only passes when MapReduce contains datapoints with indices higher than the size of the leaf_node.
Does this PR introduce a breaking change?
No
Screenshots
(Write your answer here.)
Checklist before requesting a review