Repository navigation
Fix label, axis and argument-order defects found in a targeted review - #319
Merged
Merged
Conversation
Review of IMAS.jl for the defect class behind #318 (right computation, wrong label / mirror axis / argument order). Each fix is covered by a test that fails on the previous code and uses an asymmetric case. - thermal_loads.jl mesher_heat_flux: unpacked 3 of the 4 fields returned by find_psi_last_diverted (a 4th field was inserted in Sept 2024), so psi_first_lfs_far received psi_first_open and null_within_wall received a Float. This disabled the wall-mask term that adds points in the private region around the second null (the psi window collapsed to a point), and would have raised a TypeError had that term ever selected a point. For the D3D sample and the FUSE D3D H-mode case the term selects no points, so their meshes are unchanged. Use the named fields. - fluxsurfaces.jl find_psi_last_diverted: limited-plasma branch referenced an undefined variable (UndefVarError). Use find_psi_separatrix. - fluxsurfaces.jl find_psi_separatrix: psi_sep_closed / psi_sep_open were returned swapped (closed label gave an open surface). Swap the return and make the sol.jl limited-case check use psi_sep_closed, which is what the old mislabelled value was. - equilibrium.jl symmetrize_equilibrium!: evaluation on the grid dispatched to an in-place interpolant method and threw; and the clipped Z grid was centred on (grid centre + ZA)/2, so the up-down mirror was not about the magnetic axis. Evaluate pointwise and clip symmetrically about ZA. - fluxsurfaces.jl find_x_point!: private regions classified with sum(pz) < Z0 instead of a mean, and the opposite-side and strike-point ordering tests were relative to Z = 0 instead of Z0, so a vertically shifted plasma lost its second X-point. Reference Z0 throughout. - sol.jl find_levels_from_P: the wall and dd wrappers forwarded (q, r) to the (r, q) core method; the double swap cancelled on the dd path but the wall method failed its r[1] == 0 assertion when called as documented. - geometry.jl curvature: on closed polygons the first point used a reversed entering segment (sign flipped) and the last point a zero-length leaving segment (curvature 0). Use the segments across the closing point. - profiles.jl: retrhow -> rethrow. Downstream: find_psi_separatrix consumers, symmetrize_equilibrium! (FUSE ActorEquilibrium with symmetrize=true) and curvature endpoints (FUSE limit_curvature, init_pf_active) change numerically. FUSE ActorParticleHeatFlux on the D3D H-mode case gives identical output before and after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ary_shape end points Second pass of the review (same defect classes), with asymmetric tests. - fluxsurfaces.jl find_psi_tangent_omp: separatrix half-height, outboard wall window and the two reference lines were taken about Z = 0 instead of the magnetic axis; the function threw on a vertically shifted plasma. - fluxsurfaces.jl find_psi_max: "opposite sides of the midplane" tests used products of raw Z instead of Z - ZA. - thermal_loads.jl mesher_heat_flux: the "beyond the second null" wall test was about Z = 0 instead of Z0. - profiles.jl L_H_threshold: primary x-point above/below is now judged relative to the magnetic axis, as the grad-B drift argument requires. - fluxsurfaces.jl interp_rmid_at_psi: findlast on the sign of dpsi kept every sample up to the last one with the initial sign, so a non-monotonic stretch in the middle was included and the last monotonic sample was always dropped. Keep the first monotonic stretch, including its end. - boundary.jl boundary_shape: the quadrant angle grids excluded theta = pi/2 and pi, so the top, bottom and inner-midplane points were never generated and the [2:end] trims dropped distinct points instead of duplicates (minor radius short by ~1e-4 m, uneven spacing). The closing point also differed from the first by rounding (upper vs lower triangularity), so the polygon was not exactly closed. Generate the end points, drop only shared points, snap the closing point. - sol.jl: rename levels_is_not_monotonic_in_Ip_direction to what it means. - Docstrings: cumlul_volume_integrate / cumlul_surface_integrate named the wrong function, boundary_shape documented `lkap` for `kapl`, getdata omitted the `units` field. Reviewed and left unchanged: bootstrap sign convention, orientation conventions in sol / fluxsurfaces, line_wall_2_wall index heuristics, particle window centring, x-point dedupe, relative convergence tolerances (psi-offset invariance verified), boundary_shape h-search loops, the tollZ band in mesher_heat_flux, dynamic.jl geometric_axis.z / elongation limitations. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d decreasing-psi r_mid Structural tests for the defect class behind #318 (right computation, wrong label / axis / order). Every shape and flux-surface routine is run on an asymmetric input and on the same input translated in Z, reflected up/down, with reversed point order, or with the sign of psi flipped; the outputs must transform the way their labels say. Smoke tests cover the physics files that had no test at all (equilibrium, currents, neoclassical, radiation, transport, pedestal, rf, particles, tf, pf_active, thermal_loads), including Jtor<->Jpar and JtoR<->JparB round trips. Found by the new tests: - geometry.jl intersection: several crossings on one segment of the first path were returned in second-path order instead of along the first path. find_psi_last_diverted relies on along-contour order to decide whether a surface is diverted; on the D3D sample the result differed between the plasma and its mirror image (psi_last_lfs -1.3146 vs -1.3128, converged to 1e-6) because a wall-corner clip was filtered out in one orientation. Sort within-segment crossings along the segment. - fluxsurfaces.jl interp_rmid_at_psi: a decreasing psi(R) was handed to the interpolant as decreasing knots, which FastInterpolations does not support (3 mm error at the far SOL on the sample). Reverse to increasing knots. sol.jl worked around this with its own untruncated interpolant in the psi<0 branch; both signs now go through interp_rmid_at_psi (consistency; no numerical change on the sample for either sign). - fluxsurface_extrema and luce_squareness now return NamedTuples in the same order (positional unpacking unchanged), so consumers can use names. Shared test helpers moved to test/helpers.jl. Noted for the author: flux_surfaces is not idempotent at the 2e-4 level (a second call starts from the refined boundary psi). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…egistrations - luce_squareness caught every exception and pushed 0.0, so any failure (including a genuine bug) looked like zero squareness. The only expected failure is the quadrant diagonal missing the boundary, which never happens on the sample equilibria (0 of 588 diagonals). That case is now explicit: NaN for that quadrant plus a one-time warning. Any other error propagates. - find_psi_last_diverted plotted and @show'd to the display before asserting on the intersection count; replaced with an error message carrying the intersections. find_flux formats the particle into its error message instead of @show. - Removed commented-out debug plotting/printing in intersection, inscribed_polygon, flux_surfaces, luce_squareness, find_flux, and the dead final_area / total_removed bookkeeping that only fed those prints. - Doc registrations copied from the previous block: Zcoord was registered as Particle, zohm_divertor_figure_of_merit as q_par_omp_eich, total_sources as retain_source, exponential_profile under the stale name edge_profile (undefined); beta_tor was registered twice. Every registered symbol now resolves and no section lists a name twice. Not changed: the empty catch blocks in plot.jl overlay optional diagnostic data and are intentional; the goto catch in total_fluxes skips models that lack an ion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
|
@tomneiser Have you run the fuse regression with this version of imas? |
Member
Author
|
Yes, FUSE regression passes on this version of IMAS |
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.
Fixes a set of label, mirror-axis, argument-order and Z = 0 defects of the kind found in #318, and adds tests that make this class of defect visible: every shape and flux-surface routine is run on an asymmetric input and on the same input translated in Z, reflected up/down, with reversed point order or with the sign of ψ flipped, and the outputs must transform the way their labels say.
Fixes
Could not run:
symmetrize_equilibrium!threw on grid evaluation (in-place interpolant dispatch); it also mirrored about the grid centre instead of the magnetic axis. Now evaluates pointwise and clips the Z grid symmetrically about the axis.find_psi_last_diverted, limited-plasma branch: undefined variable.find_levels_from_P, wall method: forwarded(q, r)to the(r, q)core method.find_psi_tangent_omp: wall window and separatrix half-height taken about Z = 0; threw on a vertically displaced plasma.retrhowtypo inprofiles.jl.Wrong value or label:
find_psi_separatrix:psi_sep_closed/psi_sep_openreturned swapped. Its one consumer (limited case insol.jl) adjusted accordingly.mesher_heat_flux: unpacked 3 of the 4 fields offind_psi_last_diverted, sopsi_first_lfs_farandnull_within_wallreceived the wrong values and the second-null wall mask was disabled. Uses the named fields.find_x_point!(secondary X-point lost when both nulls are on the same side of Z = 0),find_psi_max, the second-null wall mask inmesher_heat_flux, the X-point side test inL_H_threshold.intersection: several crossings on one segment of the first path were returned in second-path order.find_psi_last_divertedrelies on along-contour order, and gave different results for a plasma and its mirror image. Crossings are now ordered along the first path.interp_rmid_at_psi: kept samples up to the last one with the initial sign of dψ (non-monotonic stretches included, last sample dropped); decreasing ψ(R) was handed to the interpolant as decreasing knots, which it does not support. Keeps the first monotonic stretch and reverses to increasing knots.find_levels_from_Pnow uses it for either sign of ψ.curvatureon closed polygons: first point used a reversed entering segment, last point a zero-length one.boundary_shape: θ = π/2 and θ = π points were never generated and the closing point differed from the first by rounding. ~1e-4 m in minor radius.Also:
levels_is_not_monotonic_in_Ip_directionrenamed tolevels_is_monotonic_in_Ip_direction(that is what it holds); docstrings offind_psi_last_diverted,cumlul_volume_integrate,cumlul_surface_integrate,boundary_shape,getdatacorrected;fluxsurface_extremaandluce_squarenessreturn NamedTuples in the same order (positional unpacking unchanged).Hygiene (no numerical change)
luce_squarenesscaught every exception and pushed 0, so any failure read as zero squareness. The one expected failure (a quadrant diagonal missing the boundary, which never happens on the samples) now gives NaN with a one-time warning; other errors propagate.@showon error paths infind_psi_last_divertedandfind_fluxreplaced by error messages. Commented-out debug code and the dead bookkeeping that fed it removed.Zcoord,zohm_divertor_figure_of_merit,total_sources,exponential_profile(was registered under the undefined nameedge_profile); duplicatebeta_torremoved. Every registered symbol resolves.Tests
test/runtests_invariance.jl: translation, reflection, point-order and ψ-sign invariance ofcurvature,fluxsurface_extrema,miller_R_a_κ_δ_ζ,luce_squareness,boundary_shape(incl. round trip through the Miller fit),flux_surfaces(37 profile and boundary quantities), the sixfind_psi_*functions,sol,find_levels_from_P.test/runtests_smoke.jl: equilibrium, currents (incl. Jtor↔Jpar and JtoR↔JparB round trips), neoclassical, radiation, transport, pedestal, rf, particles, tf, pf_active and thermal_loads functions that had no test.runtests_fluxsurfaces.jl,runtests_geometry.jl,runtests_equilibrium.jl. Shared helpers intest/helpers.jl.Before / after
Blue: this branch. Orange: master. Left to right, top to bottom:
find_x_point!on the DIII-D sample shifted up by 1.5 m;symmetrize_equilibrium!cut along R = R_axis (master emulated, since it throws);curvatureof a closed circle;boundary_shapeat the inner midplane;find_psi_tangent_ompwall window on the shifted plasma;interp_rmid_at_psiknots on a non-monotonic ψ(R).Downstream
flux_surfaces, X-points, strike points, SOL and heat-flux meshes unchanged. FUSEActorParticleHeatFluxon the D3D H-mode case gives identical output.symmetrize_equilibrium!(FUSEActorEquilibriumwithsymmetrize=true),curvatureat the two end points of closed polygons (FUSElimit_curvature,init_pf_active),boundary_shapeby ~1e-4 m and 3 points,find_psi_separatrixlabels,find_psi_last_divertedon equilibria whose SOL contours are traced in the opposite direction,interp_rmid_at_psion non-monotonic or decreasing ψ(R).Not changed, noted for the authors:
flux_surfacesis not idempotent (a second call on processed data moves q by 2e-4); the midplane refinement band inmesher_heat_fluxis defined in the device frame.No version bump.
Pkg.test()green.🤖 Generated with Claude Code