minor fix + test (test_tree_of_boxes)#118
Conversation
| refine_parents_per_child = np.empty( | ||
| (nchildren, len(refine_parents)), dtype=np.intp) | ||
| refine_parents_per_child[:] = refine_parents.reshape(-1) | ||
| refine_parents_per_child = refine_parents_per_child.reshape(-1) | ||
| # Assign the parent box number for each new child box. | ||
| refine_parents_per_child = np.repeat(refine_parents, nchildren) |
There was a problem hiding this comment.
Is this correct? The before code looks more like an np.tile.
There was a problem hiding this comment.
Ah, I see, this was a bug fix! Fair enough.
| parent_has_id = box_parent_ids >= 0 | ||
| box_parent_ids[parent_has_id] = old_to_new[box_parent_ids[parent_has_id]] | ||
| child_has_id = box_child_ids != 0 | ||
| box_child_ids[child_has_id] = old_to_new[box_child_ids[child_has_id]] |
There was a problem hiding this comment.
I'm not sure it's clear to me why the above box_parent_ids = tob.box_parent_ids[neworder] isn't enough. Maybe add a little comment to this block to explain what it's trying to do?
| box_levels[coarsen_peers] = np.inf | ||
| box_levels[coarsen_peers] = np.iinfo(box_levels.dtype).max |
There was a problem hiding this comment.
How did this work before? I thought numpy won't let you convert inf to an integer like this.
>>> x[2] = np.inf
Traceback (most recent call last):
File "<console>", line 1, in <module>
OverflowError: cannot convert float infinity to integerThere was a problem hiding this comment.
Yeah, you are right. I can not find any test for coarsening, so I guess this was never run.
|
Thank you for the review, Alex! I should have included more context in the PR description. Please let me know if anything else needs to be clarified or changed. |
Looks good to me now! Thanks for all the helpful explanations. |
2 participants
Fixes
(
_apply_refine_flags_without_sorting): refining multiple boxes in one call gave the new children wrong parent IDs.refine_parents_per_childwas built(nchildren, len(refine_parents))and flattened row-major (np.tile). Butbox_children[:, refine_parents]numbers the new boxes in one contiguous block per parent. Now usesnp.repeat(refine_parents, nchildren).(
_sort_boxes_by_level): reordered the box arrays byneworderbut didn't update the IDs stored inbox_parent_idsandbox_child_ids, which still referred to the old numbering. Sorting needs two steps - rearrange the boxes into their new positions, and remap the stored IDs to the new numbering.(
_apply_coarsen_flags,_sort_and_prune_deleted_boxes):box_levelsis an integer array, so usingnp.infraisesOverflowError.coarsen_peershad a stray.reshape(-1)that flattened the(2**dim, n_sources), so coarsen_exec_flags = np.all(coarsen_peer_is_leaf, axis=0) collapsed to a single scalar.coarsen_exec_flags != coarsen_flagswas wrong since they have different sizes.Tests are included for the above fixes.