Conversation
Formating is an issue for another PR
There was a problem hiding this comment.
Pull request overview
Fixes Obstruction.get_nearest_patch so it correctly returns the nearest boundary patch (instead of frequently defaulting to the same patch), and adds an acceptance test to validate orientation selection around an obstruction (per Issue #84).
Changes:
- Reworked
get_nearest_patchminimum-distance tracking and tie handling. - Added an acceptance test covering nearest-patch orientation selection for several probe points.
- Made
test_bndfuse a path derived from the test file location.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
fdsreader/bndf/obstruction.py |
Updates nearest-patch selection logic to track only true minima and introduces deterministic tie sorting. |
tests/acceptance_tests/test_bndf.py |
Adds acceptance coverage for get_nearest_patch and adjusts case-path resolution for BNDF tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_nearest_patch(self, x: float = None, y: float = None, z: float = None): | ||
| """Gets the patch of the :class:`SubObstruction` that has the least distance to the given point. | ||
| If there are multiple patches with the same distance, a random one will be selected. | ||
| """ |
There was a problem hiding this comment.
The docstring says that if multiple patches have the same distance, a random one will be selected, but the implementation is deterministic (it sorts/takes the first element). Please update the docstring to describe the actual tie-breaking behavior (or implement true randomness if that is the intended contract).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Addresses two bugs in the original get_nearest_patch. (#84)
@JSoini: Could you check your simulation with this PR?