rtree bulk query - #5247
Conversation
ef7bc37 to
4dd30cc
Compare
ce57231 to
d8355c5
Compare
d8355c5 to
db046b3
Compare
use main branch firedrake-rtree
- Check cells ignore list before computing reference coordinates - Reduce code duplication
7f7152b to
d97a6c8
Compare
connorjward
left a comment
connorjward
left a comment
There was a problem hiding this comment.
Seems pretty much fine
| @@ -651,6 +651,8 @@ def single_eval(x, buf): | |||
| buf.ctypes.data_as(c_void_p)) | |||
| if err == -1: | |||
There was a problem hiding this comment.
Is this still right? Don't we want to check cell_out?
There was a problem hiding this comment.
This is the old Function.at code which calls a different C function defined in pointeval_utils. I suppose the rtree error path shouldn't return -1 which is what the "cell not found" error path returns.
| *cell_out = -1; | ||
| for (size_t i = 0; i < nids; ++i) { | ||
| if (ids[i] > (int64_t)PETSC_MAX_INT) { | ||
| return PETSC_ERR_ARG_OUTOFRANGE; |
There was a problem hiding this comment.
Add comment explaining why this might happen. It isn't obvious to me.
There was a problem hiding this comment.
And if this is some sort of indicator that things aren't found wouldn't -1 be a better choice?
There was a problem hiding this comment.
This is just checking that casting the ids from int64 to PetscInt is safe (for 32 bit petsc builds). Since the ids are mesh cell ids it should always be safe
There was a problem hiding this comment.
Ah I see. Definitely needs a comment but makes sense that it's there.
| return cells, Xs, ref_cell_dists_l1 | ||
|
|
||
| @PETSc.Log.EventDecorator() | ||
| def _c_locator(self, tolerance=None): |
There was a problem hiding this comment.
Slightly out of scope but could you add a docstring/comments? This is a really complicated piece of code.
| if (locate_err != PETSC_SUCCESS) {{ | ||
| rtree_free_ids(candidate_ids, candidate_offsets[npoints]); | ||
| rtree_free_offsets(candidate_offsets, npoints + 1); | ||
| return locate_err; | ||
| }} |
There was a problem hiding this comment.
More natural to
| if (locate_err != PETSC_SUCCESS) {{ | |
| rtree_free_ids(candidate_ids, candidate_offsets[npoints]); | |
| rtree_free_offsets(candidate_offsets, npoints + 1); | |
| return locate_err; | |
| }} | |
| if (locate_err != PETSC_SUCCESS) {{ | |
| break; | |
| }} |
?
Generally in C I think it is bad style to exit early because cleanup is hard
There was a problem hiding this comment.
If we just break then this function will return 0 and silently drop the error? The block as it is has all the cleanup we need to do in it.
There was a problem hiding this comment.
I mean to change the return 0 at the end of the function to return locate_err. This isn't critical, but I think it's tidier/better practice.
|
I assume this is no longer a draft? |
2 participants
locate.cto reduce code duplication. We now also check thecells_ignorearray before computing the reference coordinates.Requires a new release of firedrake-rtree.