Skip to content

3D Vacuum Part 1: Reorganizing vacuum file structure and force_wv_symmetry bugfix#145

Merged
logan-nc merged 2 commits intodevelopfrom
jmh/2D_vacuum_part1
Feb 6, 2026
Merged

3D Vacuum Part 1: Reorganizing vacuum file structure and force_wv_symmetry bugfix#145
logan-nc merged 2 commits intodevelopfrom
jmh/2D_vacuum_part1

Conversation

@jhalpern30
Copy link
Collaborator

This is part of a series of PRs for 3D vacuum that I am breaking up to be more manageable. Starting off is two things.

TL;DR - I changed file names + moved some functions around, and fixed a small bug. @logan-nc this should be a quick review, basically just if you agree with my file naming.

File reorganization

Removes Vacuum...jl naming convention, since this isn't what we did for DCON and isn't required for Julia standard. Furthermore, VacuumInternals.jl was massive and contained a bunch of unrelated functionality.

New proposed structure. NO CODE WAS CHANGED in this process, only moved from file to file.

  • DataTypes.jl
    • Holds: input/geometry structs and geometry initialization helpers.
  • MathUtils.jl
    • Holds: general math utilities (Fourier transforms, interpolation/splines, arc‑length reparameterization) after extracting the kernel. For now, also the GPEC vacuum field functions, which don't work and can be moved later.
    • distribute_to_equal_arc_grid was moved from DataTypes.jl to here
  • Kernel2D.jl
    • new home for 2D kernel routines formerly in VacuumInternals
    • Holds: 2D toroidal Green’s‑function kernel, and all related functionality for toroidal Green's functions, elliptical integrals, and Legendre polynomials.
  • (Coming soon) Kernel3D.jl
    • 3D analogy of Kernel2D.jl

BUGFIX - force_wv_symmetry was not working properly

I chose to include this in this PR since it slightly changes the outputs of the vacuum code, and will make comparing PRs to develop annoying if I tried to do them all at once.

The original fortran for this section is

      IF ( lsymz ) THEN

         do 601 l1 = 1, jmax1
            do 600 l2 = l1, jmax1
               vacmat(l1,l2) = 0.5 * ( vacmat(l1,l2)+vacmat(l2,l1) )
               vacmti(l1,l2) = 0.5 * ( vacmti(l1,l2)-vacmti(l2,l1) )
               rmatr(l1,l2) = 0.5 * ( rmatr(l1,l2)+rmatr(l2,l1) )
 600        continue
 601     continue
         do 621 l1 = 1, jmax1
            do 620 l2 = l1, jmax1
               vacmat(l2,l1) = vacmat(l1,l2)
               IF ( l1 /= l2 ) vacmti(l2,l1) = - vacmti(l1,l2)
               IF ( L1 == L2 ) vacmti(l2,l1) = 0.0
               rmatr(l2,l1) = rmatr(l1,l2)
 620        continue
 621     continue
      ENDIF

However, our Julia conversion was

    force_wv_symmetry && begin
        for l1 in 1:mpert
            for l2 in l1:mpert
                vacmat[l1, l2] = 0.5 * (vacmat[l1, l2] + vacmat[l2, l1])
                vacmti[l1, l2] = 0.5 * (vacmti[l1, l2] - vacmti[l2, l1])
            end
        end
    end

we clearly left out the second do loop here. Adding this second loop in Julia is equivalent to just calling the hermitianpart!() function, which allows a very simple line

force_wv_symmetry && hermitianpart!(wv)

As documentation that these do the same thing:
image
and we can see that these matrices are in fact Hermitian now as expected, which is not occurring in the existing code.

@jhalpern30 jhalpern30 requested a review from logan-nc February 6, 2026 14:06
@jhalpern30 jhalpern30 self-assigned this Feb 6, 2026
@jhalpern30 jhalpern30 added bug Something isn't working Cleanup and Clarifications labels Feb 6, 2026
Copy link
Collaborator

@logan-nc logan-nc left a comment

Choose a reason for hiding this comment

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

This is going to cause me headaches for #119 but you are doing the right thing by chunking work into digestible PRa so I am going to reward that and merge - dealing with the headache on the other end.

Please note that an issue should be opened that requests we move as many basic "mathematical utilities" to a shared top level directory. I've created it in #119, but after that is merged the task should be to go through each directory (ForceFreeStates, Vacuum, etc.) and pull things out as appropriate. Part of this will be migrating some things. For example, we don't need to reinvent FFTs for vacuum - we can use a standard package.

@logan-nc logan-nc merged commit 7abc2b2 into develop Feb 6, 2026
5 checks passed
@logan-nc logan-nc deleted the jmh/2D_vacuum_part1 branch February 6, 2026 14:33
@claude
Copy link

claude bot commented Feb 6, 2026

PR Review

This PR reorganizes the Vacuum module file structure and fixes a bug in the symmetry enforcement. The changes are well-structured and the bugfix is mathematically correct.

File Reorganization

Positives:

  • The new structure is clearer and more logical:
    • DataTypes.jl: geometry structs and initialization
    • Kernel2D.jl: 2D toroidal Green's function kernels
    • MathUtils.jl: general math utilities
  • Removing the Vacuum... prefix aligns with the DCON module convention
  • Breaking up the massive VacuumInternals.jl file improves maintainability
  • This organization will scale better when Kernel3D.jl is added

Considerations:

  • All functions appear to be moved verbatim (no code changes during reorganization), which is the right approach
  • The MathUtils.jl file is now 536 lines - still manageable, but may benefit from further subdivision in future work

Bugfix: force_wv_symmetry

The bug: The Julia conversion was missing the second nested loop from the Fortran code that copies the upper triangle to the lower triangle.

The fix: Replacing the incomplete implementation with hermitianpart!(wv) is correct and cleaner. This function computes (A + A')/2 in-place, which matches the Fortran logic:

  1. First loop: compute upper triangle as (vacmat[l1,l2] + vacmat[l2,l1])/2
  2. Second loop: copy to lower triangle vacmat[l2,l1] = vacmat[l1,l2]

Issues identified:

  1. Incorrect symmetrization for complex matrices (line 307): The current fix uses hermitianpart!, which computes (A + A^H)/2 where A^H is the conjugate transpose. However, the Fortran code at Vacuum.jl:307 only symmetrizes the real and imaginary parts separately without conjugation:

    • vacmat[l2,l1] = vacmat[l1,l2] (real part, symmetric)
    • IF ( l1 /= l2 ) vacmti[l2,l1] = -vacmti[l1,l2] (imaginary part, antisymmetric)

    For a complex matrix, this means the Fortran creates a matrix where M[i,j] = conj(M[j,i]) (Hermitian), but hermitianpart! computes (M + M')/2 without conjugation. You should verify whether hermitianpart! is the correct function here or if you need a custom implementation that matches the Fortran behavior exactly.

  2. Missing rmatr handling (line 307): The Fortran code symmetrizes three matrices: vacmat, vacmti, and rmatr. The Julia code only handles wv = complex(vacmat, vacmti) but doesn't mention rmatr. Is rmatr no longer used, or is this a potential issue?

  3. Ordering change (lines 303-307): The new code creates the complex matrix first, then symmetrizes. The old code symmetrized the real/imaginary parts separately, then combined them. While mathematically this should be equivalent for Hermitian symmetrization, verify this doesn't introduce numerical differences.

Minor Issues

  1. Line 917 in MathUtils.jl: del_Z calculation uses plasma_surf.x (R-coordinates) instead of plasma_surf.z (Z-coordinates). This appears to be an existing bug carried over from the original code:

    del_R = 1e-5 * (maximum(plasma_surf.x) - minimum(plasma_surf.x))
    del_Z = 1e-5 * (maximum(plasma_surf.x) - minimum(plasma_surf.x))  # Should use plasma_surf.z
  2. Test coverage: Consider adding a test that verifies the force_wv_symmetry fix produces Hermitian matrices, similar to the validation shown in the PR description screenshot.

Recommendations

  1. Critical: Verify that hermitianpart! produces the same result as the Fortran code for complex matrices. If not, implement a custom symmetrization function.

  2. Investigate whether rmatr should be handled.

  3. Fix the del_Z bug in MathUtils.jl:917 (or file a separate issue if this is existing tech debt).

  4. Add a test case that validates the symmetry properties of the output matrix.

The file reorganization is solid. The bugfix approach is on the right track, but needs verification of the complex matrix symmetrization behavior before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Cleanup and Clarifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants