Repository navigation
r.sim.water: Fix flaky buffer overflow and OpenMP concurrency issues - #7515
anzalkabeer wants to merge 3 commits into
Conversation
Add a second tab to the Query Results dialog that sends the queried map data (elevation, land use, aspect, vector attributes, etc.) to an LLM (Gemini or OpenAI) and displays a human-readable terrain description. Implementation uses only Python standard library (urllib.request, threading) to avoid new dependencies. API calls run in a background thread with results posted back via wx.CallAfter. Larger use of AI was involved in writing this feature.
- Add missing bounds check exit (continue) in hydro.c loop - Replace thread-unsafe G_drand48() in OpenMP threads with thread-safe LCG PRNG - Prevent concurrency data races on shared grids by adding OpenMP atomic and critical sections
github-actions
Bot
added
GUI
anzalkabeer
changed the title
anzalkabeer
changed the title
|
Hi @anzalkabeer, Nice to see the buffer-overflow fix and the thread-safe PRNG improvements. One thing I noticed is that this PR also modifies Separately, I've been working on the testing side in #7506. That work adds a synthetic, property-based pytest that validates physical behavior rather than exact cell values, making it robust to thread scheduling and other sources of nondeterminism. I'd be happy to help verify your fix against those tests, or check whether parallel runs are now deterministic, if that would be useful. |
|
Try to create a branch first with a change for each PR. Otherwise, you cannot have two PRs in the same repo for the same branch. You’ll probably want to update your main branch of your fork to this repo many times in the future, and you’ll be stuck if an open PR uses it. Also, for us (or me) we’ll receive CI updates on runs in your fork if we merge your main branch as a PR, as it’ll count like if we are participants to your fork. It already happened, and it’s just a little bit of a disturbance. So, search a bit how to do a branch of your fork from main, then either rebase interactively to remove the extra commit or learn how to cherry pick the commit you want to a new branch, learn how to keep your fork in sync on GitHub, and then try resubmitting what you intended as a fresh PR. It’s not so fun trying to learn these quirks of git and GitHub in the beginning, but it’ll be useful for you. |
|
No worries @anzalkabeer, the fix itself looked solid. Once you reopen it on a clean branch, I'd be glad to test it against the #7506 tests and check that parallel runs come out deterministic now. Looking forward to it. |
|
@Valyrian-Code i have reopened a fresh branch for the same fix and also have dropped the llm api call and _format_query_data_as_text , please take a look into it . |
|
Thanks @anzalkabeer, and good call dropping the LLM/query.py changes. I had a look at the fresh PR and left some notes on #7521: the CI failures look like real causes rather than rate limiting (the new PRNG changes r.sim.water's output so it no longer matches the committed reference rasters, and Super Linter is failing on an editorconfig issue). Details are in that thread. |
r.sim.water: buffer overflow #4810:
fixed the out of bounds walkers , also the threads are safe from random number generation. by adding a custom PRNG utilizing a thread indexed array . please look into my fixes