Fix hashtable.query by checking for both roots in index_search - #2769
Draft
erikvansebille wants to merge 1 commit into
Draft
Fix hashtable.query by checking for both roots in index_search#2769erikvansebille wants to merge 1 commit into
erikvansebille wants to merge 1 commit into
Conversation
Member
Author
|
UPDATE: Actually, this fix turns out not to be needed. I had accidentally swapped the definition of the then the index_search works also without the fix in this PR So the question is then: should we keep this fix? Or better to not merge it into main? Note that the code does become slower. Running with this PR, a simulation took 65 minutes, while it took 41 minutes on main with the axes-swapping |
This file contains hidden or 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
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.
Description
While working on the Delft3D with NaNs for lon//lat, I realised that the issue wasn't the creation of the hashtable itself (that works fine with NaNs for lon/lat points), but that the point-in-cell test always returned False, even if a point was clearly in a cell.
After some further digging, I discovered that the issue was in
index_search._bilinear_inverse(), which did not return the correct barycentric coordinatesWith some help from an LLM (see prompt below), I then found the issue and a solution:
The function produces two quadratic roots for eta, but the current implementation always selects the
(−bb+det)/(2a)solution. It should of course also check the(−bb-det)/(2a)solution.This PR implements that. And now the hashtable search works on the Delft3D grid!
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
"I use the _bilinear_inverse function here to find out the baricentric coordinates of a point_in_cell check. However, it does not seem to be working because if I input px: [[39878.2421875 39849.01953125 39819.69921875 39790.2734375 ]
[40011.484375 39982.2109375 39952.8359375 39923.3515625 ]
[39982.2109375 39952.8359375 39923.3515625 39893.75 ]
[39849.01953125 39819.69921875 39790.2734375 39760.71875 ]], py: [[450116.90625 450081.46875 450045.90625 450010.3125 ]
[450014.3125 449978.875 449943.3125 449907.6875 ]
[449978.875 449943.3125 449907.6875 449871.90625]
[450081.46875 450045.90625 450010.3125 449974.5625 ]], xq: [39900. 39900. 39900. 39900.], yq: [450000. 450000. 450000. 450000.]
then I get xsi: [-1481.18252686 -1380.11892455 -762.69672299 -868.69661282], eta: [ 4291.41378809 3983.77629991 6657.93389165 10363.58362922], but none of these are between 0 and 1, as I would expect because the point lays within one of the cells"