Don't default particle_id_type in get_kernel_info#114
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the internal defaulting of particle_id_dtype in FMMTraversalBuilder.get_kernel_info and makes callers pass an explicit dtype, aligning kernel compilation settings with the tree’s declared particle_id_dtype.
Changes:
- Make
particle_id_dtypea required argument toget_kernel_infoand remove theNone -> int32fallback. - Update traversal generation to pass
tree.particle_id_dtypedirectly instead of usinggetattr(..., None).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| knl_info = self.get_kernel_info( | ||
| dimensions=tree.dimensions, | ||
| particle_id_dtype=getattr(tree, "particle_id_dtype", None), | ||
| particle_id_dtype=tree.particle_id_dtype, | ||
| box_id_dtype=tree.box_id_dtype, |
There was a problem hiding this comment.
__call__ now unconditionally accesses tree.particle_id_dtype. TreeOfBoxes does not define this attribute (see boxtree/tree.py), and there is an existing test that calls FMMTraversalBuilder(actx)(actx, tob). This change will raise AttributeError for TreeOfBoxes and break traversal generation for box-only trees. Consider restoring a fallback here (e.g. derive a dtype from tree.box_id_dtype or a fixed int dtype for box-only trees), or ensure TreeOfBoxes provides a particle_id_dtype attribute so traversal kernels can be built consistently.
| def get_kernel_info(self, *, | ||
| dimensions: int, | ||
| particle_id_dtype: np.dtype[np.integer[Any]] | None, | ||
| particle_id_dtype: np.dtype[np.integer[Any]], | ||
| box_id_dtype: np.dtype[np.integer[Any]], | ||
| coord_dtype: np.dtype[np.floating[Any]], | ||
| box_level_dtype: np.dtype[np.integer[Any]], | ||
| max_levels: int, | ||
| sources_are_targets: bool, | ||
| sources_have_extent: bool, | ||
| targets_have_extent: bool, | ||
| extent_norm: ExtentNorm | None, | ||
| source_boxes_has_mask: bool, | ||
| source_parent_boxes_has_mask: bool, | ||
| debug: bool = False) -> _KernelInfo: |
There was a problem hiding this comment.
get_kernel_info previously accepted particle_id_dtype=None and selected a default. With the parameter now required and the defaulting logic removed, callers that intentionally relied on the previous behavior (notably traversal generation for TreeOfBoxes, which historically had no particle_id_dtype) will fail unless they supply a dtype. If the goal is to avoid defaulting inside get_kernel_info, consider moving the default selection to the caller (or making it explicit via a required particle_id_dtype attribute on all supported tree types) to preserve existing functionality.
No description provided.