Account for spherical face bulging during hashgrid construction and point in cell checks - #2883
Account for spherical face bulging during hashgrid construction and point in cell checks#2883wyatt-fluidnumerics wants to merge 20 commits into
Conversation
…_spherical_uxgrid_face_bounds_contain_face_interior to unit test _spherical_triangle_bounds
…truction as the index_search test, and removed xfail
…tion, removed xfail and recommented test_sample_points_inside_faces
for more information, see https://pre-commit.ci
…nto bugfix/issue-2878
Making sure that this tutorial is executed again, after the fix in #2883
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good; one comment and one question below
| # Construcuct a matrix of face positions to solve for the | ||
| # byarycentric coordinates of each point, and then convert to coords = w0,w1,w2 | ||
| # such that w0*v0 + w1*v1 + w2*v2 = point, normalized to sum to 1. | ||
| face_matrix = np.stack( |
There was a problem hiding this comment.
Is this the most efficient implementation? Does using np.stack here (and later in this PR) not lead to a similar performance loss as #2846? Or are the arrays so small here that that doesn't matter?
There was a problem hiding this comment.
Hmm, yeah, maybe preallocating face_matrix and concatenating into this array would be better? I wonder if concatenating in Python concats by reference or by value
There was a problem hiding this comment.
Looks like we'd have to use the out= argument with concatenate (or stack) to avoid additional allocation costs, see https://numpy.org/devdocs/reference/generated/numpy.concatenate.html and https://numpy.org/doc/stable/reference/generated/numpy.stack.html
Source code for stack shows it just calls concatenate under the hood .
Neither avoids the copy cost here though. Buffers are copied . Only numpy only route I see possible here is to use an array view to avoid copies.
There was a problem hiding this comment.
Thanks for digging into this, @fluidnumericsJoe. I don't think we need to go too deep into optimisation of this statement specifically before we know whether it is a bottle-neck or not. So I'd avoid creating our own views. I just raised the question in case there was a low-hanging-fruit solution to make this faster
There was a problem hiding this comment.
Pre-allocating and using np.stack with the out parameter could be helpful - but that pre-allocation might need to have broader scope than just a single PIC call to be of any value now that I think of it. IMO, after thinking on this a bit, I wouldn't worry about this too much here since its a single call per vectorized PIC check.
There was a problem hiding this comment.
I updated the nested grids tutorial so that gets run again - hope that's ok?
There was a problem hiding this comment.
That's great, thank you!
… bulge for spherical structured curvilinear grids
… on spherical structured curvilinear grids
for more information, see https://pre-commit.ci
…ather than orthonomal
|
@erikvansebille @fluidnumericsJoe I've added the changes for spherical curvilinear grids. I generalized the _spherical_face_bounds function to work for both cases, and then adjusted _spherical_project_cell_and_query to use a gnomonic projection instead of orthonormal. I also changed some of the new tests to use xgrid/uxgrid parameterizations. I think things should be good to go now, but would be great to get another review on the changes! |
Description
Issue #2878 raised awareness of two separate bugs:
Both of these issues are magnified for grids that contain large cells, which is why for most of the small celled grids that Parcels is tested on, they did not show up earlier.
The fixes are the following:
The PR in this state covers the case for unstrucutred grids. Structured curvilinear grids still suffer the same bug and I have yet to implement these fixes there. I'll try and do that tomorrow so this is resolved before the v4 release!
Checklist
tutorial_nested_grids.ipynb- grid searching #2878mainfor normal development,v3-supportfor v3 support)AI Disclosure
Currently a draft PR until the spherical curvilinear fixes are in.