Reapply GK passive advection work (PR #1084) - #1102
Merged
Merged
Conversation
Antoinehoff
marked this pull request as ready for review
September 1, 2026 14:31
JunoRavin
approved these changes
Sep 1, 2026
… given in the input file. Add sample regression test (gkylcas 16cfe76398a327fa8617ed162509585f0c6c6f47). (cherry picked from commit 771c19a)
…up. We should make it so that omega_H is only computed if the field evolves. Passive eqn object is clearly not ready as the result is not as expected. (cherry picked from commit 1056915)
…x2v passive advection reg test. Tried using TS BCs in passive advection but I think there's a memory bug, need to switch to cluster with valgrind. (cherry picked from commit 9aff987)
…oken its use in bc_twistshift. Also, I think there was a bug in find_cell all along, though I don't know if it was ever triggered. Fix this bug, and adjust a tolerance in twistshift to make it work with the recently added epsilon in rect_grid_find_cell. Unit tests pass. There's still an issue with passive advection using TS BCs because the solution quickly becomes NaNs. (cherry picked from commit b3601fd)
… show that the shift at the lower boundary is OK, but the shift at the upper boundary (which -1 times the lower shift) produces NaNs. (cherry picked from commit fa5ca03)
… integrals of bc_twistshift, it was causing NaNs in the fig14 scenario. Fix boundary kernel selection in gk_collisionless_passive_flux. Now the passive advection test with TS BCs looks as expected. (cherry picked from commit 965ca1a)
…uadratic q has issues near the upper radial boundary. We'll examine this in a unit test setting, and try it with TCV geo. (cherry picked from commit 5af001f)
… delete or simplify this in the future, but it already reproduces the checkerboard pattern seen in the simulation. (cherry picked from commit ce9b41d)
…in the shift across a single cell is equal or greater to the length of the domain along y. I tried to add support for this sort of scenario but didn't succeed. I'll try again in the future. For now we must increase resolution along y, reduce shear or increase Ly so this doesn't happen. Add accepted results to CBC unit test. (cherry picked from commit 9473d77)
…n TS BC updater. The previous approach looked in periodic copies of the domain independently for each point, but this could give different periodic copies for 2-4 points that are meant to define a subcell integral, leading to error. So now we look for the correct periodic copy only once (for a given donor-target pair) and pass that to be used for looking for intersections. This fixes some checkerboarding seen in the CBC geo with Nx cells. All other unit tests continue to pass (I had to generate new cbc accepted results, but I confirmed visually that for Nx=32 the difference was machine precision. (cherry picked from commit f4a9122)
…eger. This explains a mysterious shift we were seeing when structures crossed the IMP. (cherry picked from commit c289cd3)
(cherry picked from commit ae49bfd)
…ew, so we can call it from python. (cherry picked from commit b6cac76)
…ddress, this makes it easier to call it from python. Rename num_cell_ variables in tcv reg tests so that they use Nx like all other reg tests (makes it easier to automate changes to them). (cherry picked from commit ce4c128)
…ased in its local scope. (cherry picked from commit 7545225)
…n't run at too low a res because of constraints on TS BCs. Increase tolerance in unit test to 1e-8 so it passes on Perlmutter. (cherry picked from commit 4ebcbc8)
… Perlmutter CPU. (cherry picked from commit 5a6498d)
(cherry picked from commit d17066b)
…compilers/libraries being used/linked. (cherry picked from commit 520e8b7)
(cherry picked from commit 2e9eba0)
Antoinehoff
force-pushed
the
restore_gk_passive_1084
branch
from
September 1, 2026 19:34
fd4b8c8 to
3933c44
Compare
Maxwell-Rosen
approved these changes
Sep 1, 2026
Maxwell-Rosen
left a comment
Maxwell-Rosen
left a comment
Collaborator
There was a problem hiding this comment.
Antoine explained this to me. It seems he accidently reverted these commits in a branch and they were intended to be merged.
Collaborator
Author
|
Yes, let's just wait for @manauref because its his work I messed up with 😬 |
manauref
approved these changes
Sep 1, 2026
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
4 participants
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.
This reverts commit dbc4aba that cancelled PR #1084. My mistake sorry...
This was done by cherry picking the commits that were removed. This should preserve authorship AND git blame. This dark magit was done with Claude opus 5.
We also put back #1095 that was also removed.