Add first attempt on Hash-based box search #53
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
aartiukh
reviewed
Oct 12, 2024
Comment on lines
+62
to
+64
| # discretized_coords = [] | ||
| # for coord in point: | ||
| # discretized_coords.append(int((coord + sys.float_info.epsilon) // self.search_radius)) |
| Initialize the box-based neighbors search algorithm. | ||
| :param search_radius: minimal distance for particles to become neighbors | ||
| :param epsilon: precision for distance calculation, set to 10e-8 by default | ||
| :param domain_size: upper bound for the working domain |
There was a problem hiding this comment.
not all arguments have been documented. please add all args
| self.epsilon = epsilon | ||
| self.domain_size = domain_size | ||
| self._verbose = verbose | ||
| self.box_size = cell_size if cell_size is not None else self.search_radius |
There was a problem hiding this comment.
Why we have cell_size input param? I believe it should always be calculated
Comment on lines
+27
to
+33
| self._boxes_in_row = round(self.domain_size / self.search_radius) | ||
| self._total_boxes = self._boxes_in_row ** 2 | ||
| self.hashed_box_id2points = {} | ||
| self.box_id2hash = {} | ||
| self.hash2box_id = {} | ||
| self._int_neighbor_boxes_ids = None | ||
| self._box_neighbors = self._find_neighbor_boxes() |
There was a problem hiding this comment.
Please add comments to these vars, the names are not self-explanatory.
Comment on lines
+57
to
+60
| col = int((point[0] + sys.float_info.epsilon) // self.search_radius) | ||
| row = int((point[1] + sys.float_info.epsilon) // self.search_radius) | ||
| discretized_point_coords = [col, row] | ||
| box_id = int(row * self._boxes_in_row + col) |
There was a problem hiding this comment.
Why is there code instead of docs?
| LOG.debug(message) | ||
|
|
||
| @staticmethod | ||
| def _get_hash(col: int, row: int) -> int: |
There was a problem hiding this comment.
Please add comments to all methods, including private
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.
What does this PR do?
Add hash-based box search and tests.
Issue reference
Explanation
This PR brings first attempt to implement hash-based box search for neighbors search optimization.
How to get?
git clone --recursive -b feature/blabla [email protected]:aartiukh/sph.git