Skip to content

Position map consistency check - #1091

Open
Maxwell-Rosen wants to merge 2 commits into
mainfrom
pos-map-consist
Open

Position map consistency check#1091
Maxwell-Rosen wants to merge 2 commits into
mainfrom
pos-map-consist

Conversation

@Maxwell-Rosen

@Maxwell-Rosen Maxwell-Rosen commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Claude found an inconsistency in how the non-uniform position map is constructed for the Delta B minimizing map. The cap is fixed to pivot on the region's own uniform-coordinate bounds instead of raw theta, eliminating the geometry-breaking discontinuity and making maximum_slope_at_max_B safe to enable.

The slope limiters work by asking if the map is above or below the line that passes through the inflection point of the B field, the point where the non-uniform map changes from concave down to concave up, is above or below this line. The line has a slope set by the input file, so a linear map here is the maximum/minimum allowable deformation while preserving the map structure.

image

In this image, the y-axis is the new non-uniform position and the x-axis is the uniform axis. Here, the left side would have very sparse points, so we draw a red line with a known slope and instead use the blue region, where point density is lowest. Similarly, the grid gets very compact on the right, so we draw a line and take the blue linear part to keep the grid from getting too compact.

…m_slope_at_min_B is set in every shipped example — but I traced why nobody sets maximum_slope_at_max_B: it's not an oversight, it's flatly broken. Enabling it previously produced "left-handed coordinate system, J<0" geometry failures (this exact attempt and rejection is documented in MIRROR_DEBUG_REPORT.md from an earlier debugging session).

The bug, in gyrokinetic/zero/gkyl_position_map_priv.h's position_map_constB_z_numeric: the slope-cap straight line was anchored directly on the raw uniform coordinate theta, implicitly assuming theta ≈ Theta_left/Theta_right (the physical extrema locations). That's only true exactly at a region boundary. Since dB-accumulation regions have very different widths in uniform-theta space than in physical space, and the Gaussian smoothing (gaussian_std>0, used by every shipped example) evaluates the map at theta values whose own region can be narrower than the smoothing window, the cap routinely got evaluated far outside its own region — producing wildly wrong, non-monotonic values that broke the geometry.

Fix: anchor the cap on the region's own uniform-coordinate bounds (theta_bound_lower/theta_bound_upper, derived from the already-available dB_global_lower/dB_cell/theta_dxi) instead of raw theta.

Verified:
- calc_metric.c orthonormality/left-handed-coordinate errors: hundreds → zero, with both slope caps enabled.
- Monotonicity violations in the resulting map: 570/20000 → 1/20000 (matches the noise floor of the already-working min-B-only baseline).
- ctest_position_map, ctest_gk_geometry_mirror, ctest_gk_geometry_mapc2p unit tests: pass identically before/after (the ctest_gk_geometry_tok failures are pre-existing on main, unrelated to this change).
- Added maximum_slope_at_max_B=2 to rt_gk_mirror_boltz_elc_1x2v_p1_minimal.c; a -x64 run stays smooth (constant dt≈1.236e-9) well past the 13%/t≈1.3e-6 mark where the earlier (pre-map_strength fix) version used to abort.

Not done: the full t_end=10e-6, 100-frame Tpar/Tperp/density re-validation — changing the z-grid mapping again means that should be redone before calling this a complete physics fix, not just a geometry-validity fix. The rt_gk_wham_nonuniformx_* examples have the identical gap and can now safely set maximum_slope_at_max_B too, but I left those untouched since they're outside this session's active work. Noted both in memory.
@Antoinehoff

Copy link
Copy Markdown
Collaborator

Hey Max, I'm not very familiar with that part of the code, could you illustrate more where it is used and why this fix matters? Also are Mana and I assignees or reviewers?

@Maxwell-Rosen

Copy link
Copy Markdown
Collaborator Author

I'm using these slope limiters to make sure the non-uniform mapping doesn't become too steep. This is a concern near the mirror throat, midplane, and near the simulation boundaries, where dB/dz is near zero. I'm using these position maps in my mirror simulations.

// upper instead so the cap is evaluated in the same coordinate as theta
// everywhere in the region (this matters most when a caller, e.g. the
// Gaussian smoothing quadrature, evaluates theta values whose own region
// is much narrower or wider in Theta-space than in theta-space).

@Antoinehoff Antoinehoff Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you shorten this comment and make sure it answers to the "what" but not to the "how"?
Since it seems you are changing the method here, could you provide a detailed explanation in the PR description on what this algorithm does now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the bulk of this comment, shortening it to just "Set a minimum cell size on the edges."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, could you also provide more explanation about how this is done in the PR description for documentation purposes?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. It's hard to describe this without an image, so I put a crude MS Paint sketch to explain what this feature is doing.

@Antoinehoff
Antoinehoff self-requested a review August 3, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants