refactor: modularize libneo_coordinates with proper OOP architecture

[krystophny]

User description

Summary

• Refactor libneo_coordinates from monolithic merged file to clean module hierarchy
• Fix fpm.toml invalid dependency syntax for FPM compatibility
• Add bind.toml for fpm-bind Python binding generation

Changes

Replace libneo_coordinates_merged.f90 (1164 lines) with proper OOP modules:

Module	Purpose
libneo_coordinates_base	Abstract coordinate_system_t type + constants
libneo_coordinates_vmec	VMEC coordinate system implementation
libneo_coordinates_geoflux	Geoflux coordinate system implementation
libneo_coordinates_chartmap	Chartmap coordinate system implementation
libneo_coordinates_validator	Chartmap file validation utilities
libneo_coordinates_file_detection	Reference coordinate file type detection
libneo_coordinates	Facade module re-exporting all public API

Test plan

• All 70 CMake tests pass
• FPM build succeeds
• fpm-bind Python binding generation works

---

PR Type

Enhancement

---

Description

• Refactor monolithic libneo_coordinates into modular OOP architecture

  • Split 1164-line merged file into 7 focused modules with clear responsibilities
  • Create base module with abstract coordinate_system_t type and constants
  • Implement separate modules for VMEC, Geoflux, and Chartmap coordinate systems

• Fix FPM build compatibility issues

  • Correct invalid dependency syntax in fpm.toml using external-modules
  • Add bind.toml for fpm-bind Python binding generation support

• Update build configuration for new modular structure

  • Modify CMakeSources.in to reference individual modules instead of merged file
  • Maintain backward compatibility through facade module re-exporting public API

---

Diagram Walkthrough

flowchart LR
  merged["libneo_coordinates_merged.f90<br/>1164 lines"]
  base["libneo_coordinates_base.f90<br/>Abstract types & constants"]
  vmec["libneo_coordinates_vmec.f90<br/>VMEC implementation"]
  geoflux["libneo_coordinates_geoflux.f90<br/>Geoflux implementation"]
  chartmap["libneo_coordinates_chartmap.f90<br/>Chartmap implementation"]
  validator["libneo_coordinates_validator.f90<br/>File validation"]
  detection["libneo_coordinates_file_detection.f90<br/>File type detection"]
  facade["libneo_coordinates.f90<br/>Facade module"]
  
  merged -- "refactored into" --> base
  merged -- "refactored into" --> vmec
  merged -- "refactored into" --> geoflux
  merged -- "refactored into" --> chartmap
  merged -- "refactored into" --> validator
  merged -- "refactored into" --> detection
  
  base -- "imported by" --> facade
  vmec -- "imported by" --> facade
  geoflux -- "imported by" --> facade
  chartmap -- "imported by" --> facade
  validator -- "imported by" --> facade
  detection -- "imported by" --> facade

Loading

File Walkthrough

	Relevant files
Refactoring	7 files libneo_coordinates.f90 Convert to facade module re-exporting public API                  +18/-256 libneo_coordinates_vmec.f90 Convert submodule to standalone module                                      +26/-6    libneo_coordinates_geoflux.f90 Convert submodule to standalone module                                      +34/-16  libneo_coordinates_chartmap.f90 Convert submodule to standalone module                                      +48/-27  libneo_coordinates_validator.f90 Convert submodule to standalone module                                      +7/-3      libneo_coordinates_file_detection.f90 Convert submodule to standalone module                                      +10/-3    libneo_coordinates_merged.f90 Delete monolithic merged file                                                        +0/-1164
Enhancement	2 files libneo_coordinates_base.f90 New module with abstract types and constants                          +135/-0  bind.toml Add fpm-bind Python binding configuration                                +7/-0
Configuration changes	1 files CMakeSources.in Update build sources for modular structure                              +7/-1
Bug fix	1 files fpm.toml Fix FPM dependency syntax and add external modules              +7/-5

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Hard stop error: The new geoflux_from_cyl implementation unconditionally terminates the process via error stop and even initializes ierr = 0, preventing graceful error propagation to callers. Referred Code subroutine geoflux_from_cyl(self, xcyl, u, ierr) class(geoflux_coordinate_system_t), intent(in) :: self real(dp), intent(in) :: xcyl(3) real(dp), intent(out) :: u(3) integer, intent(out) :: ierr associate(dummy => self) end associate associate(dummy2 => xcyl) end associate u = 0.0_dp ierr = 0 error stop "geoflux_from_cyl: not implemented" end subroutine geoflux_from_cyl Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error details exposed: The error stop "geoflux_from_cyl: not implemented" message exposes internal function naming/implementation details and it is unclear from the diff whether this can reach end users or is confined to internal logs/tests. Referred Code subroutine geoflux_from_cyl(self, xcyl, u, ierr) class(geoflux_coordinate_system_t), intent(in) :: self real(dp), intent(in) :: xcyl(3) real(dp), intent(out) :: u(3) integer, intent(out) :: ierr associate(dummy => self) end associate associate(dummy2 => xcyl) end associate u = 0.0_dp ierr = 0 error stop "geoflux_from_cyl: not implemented" end subroutine geoflux_from_cyl Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect derivative calculation logic Add a check for self%zeta_convention == CYL in chartmap_eval_cart_der before using the spl_rz path, as its derivative calculation is only valid for the cylindrical zeta convention. src/coordinates/libneo_coordinates_chartmap.f90 [268-302] subroutine chartmap_eval_cart_der(self, u, vals, dvals) class(chartmap_coordinate_system_t), intent(in) :: self real(dp), intent(in) :: u(3) real(dp), intent(out) :: vals(3) real(dp), intent(out) :: dvals(3, 3) real(dp) :: rz(2), drz(3, 2) real(dp) :: cph, sph - if (self%has_spl_rz) then + if (self%has_spl_rz .and. self%zeta_convention == CYL) then cph = cos(u(3)) sph = sin(u(3)) call evaluate_batch_splines_3d_der(self%spl_rz, u, rz, drz) vals(1) = rz(1)*cph vals(2) = rz(1)*sph vals(3) = rz(2) dvals(1, 1) = drz(1, 1)*cph dvals(2, 1) = drz(1, 1)*sph dvals(3, 1) = drz(1, 2) dvals(1, 2) = drz(2, 1)*cph dvals(2, 2) = drz(2, 1)*sph dvals(3, 2) = drz(2, 2) dvals(1, 3) = drz(3, 1)*cph - rz(1)*sph dvals(2, 3) = drz(3, 1)*sph + rz(1)*cph dvals(3, 3) = drz(3, 2) else call evaluate_batch_splines_3d_der(self%spl_cart, u, vals, dvals) end if end subroutine chartmap_eval_cart_der [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a subtle but critical bug in the derivative calculation that occurs under a specific condition (zeta_convention is not CYL) and provides a correct fix to prevent incorrect results.	High
General	Avoid redundant basis vector calculations To optimize coordinate_system_cov_to_cart, call self%covariant_basis once, then compute the metric tensor and its inverse from the resulting basis vectors within the subroutine to avoid redundant calculations. src/coordinates/libneo_coordinates_base.f90 [108-121] subroutine coordinate_system_cov_to_cart(self, u, v_cov, v_cart) class(coordinate_system_t), intent(in) :: self real(dp), intent(in) :: u(3) real(dp), intent(in) :: v_cov(3) real(dp), intent(out) :: v_cart(3) real(dp) :: e_cov(3, 3), g(3, 3), ginv(3, 3), sqrtg real(dp) :: v_ctr(3) + real(dp) :: det + integer :: i, j - call self%metric_tensor(u, g, ginv, sqrtg) + call self%covariant_basis(u, e_cov) + + do i = 1, 3 + do j = 1, 3 + g(i, j) = dot_product(e_cov(:, i), e_cov(:, j)) + end do + end do + + det = g(1,1)*(g(2,2)*g(3,3) - g(2,3)*g(3,2)) & + - g(1,2)*(g(2,1)*g(3,3) - g(2,3)*g(3,1)) & + + g(1,3)*(g(2,1)*g(3,2) - g(2,2)*g(3,1)) + + sqrtg = sqrt(abs(det)) + + ginv(1,1) = (g(2,2)*g(3,3) - g(2,3)*g(3,2))/det + ginv(1,2) = (g(1,3)*g(3,2) - g(1,2)*g(3,3))/det + ginv(1,3) = (g(1,2)*g(2,3) - g(1,3)*g(2,2))/det + ginv(2,1) = (g(2,3)*g(3,1) - g(2,1)*g(3,3))/det + ginv(2,2) = (g(1,1)*g(3,3) - g(1,3)*g(3,1))/det + ginv(2,3) = (g(1,3)*g(2,1) - g(1,1)*g(2,3))/det + ginv(3,1) = (g(2,1)*g(3,2) - g(2,2)*g(3,1))/det + ginv(3,2) = (g(1,2)*g(3,1) - g(1,1)*g(3,2))/det + ginv(3,3) = (g(1,1)*g(2,2) - g(1,2)*g(2,1))/det + v_ctr = matmul(ginv, v_cov) - call self%covariant_basis(u, e_cov) v_cart = matmul(e_cov, v_ctr) end subroutine coordinate_system_cov_to_cart Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a redundant calculation where covariant_basis is called twice and proposes an effective optimization by calculating the metric tensor directly, improving performance.	Low
	Remove unused code from stub Remove the unused associate blocks and variable initializations from the geoflux_from_cyl stub function to improve code clarity. src/coordinates/libneo_coordinates_geoflux.f90 [124-138] subroutine geoflux_from_cyl(self, xcyl, u, ierr) class(geoflux_coordinate_system_t), intent(in) :: self real(dp), intent(in) :: xcyl(3) real(dp), intent(out) :: u(3) integer, intent(out) :: ierr - associate(dummy => self) - end associate - associate(dummy2 => xcyl) - end associate - - u = 0.0_dp - ierr = 0 error stop "geoflux_from_cyl: not implemented" end subroutine geoflux_from_cyl [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies and removes unused code from a stub function, which improves code clarity and maintainability.	Low
	Remove redundant associate Remove the empty associate block from geoflux_evaluate_cyl to improve code clarity. src/coordinates/libneo_coordinates_geoflux.f90 [27-36] subroutine geoflux_evaluate_cyl(self, u, x) class(geoflux_coordinate_system_t), intent(in) :: self real(dp), intent(in) :: u(3) real(dp), intent(out) :: x(3) - associate(dummy => self) - end associate - call geoflux_to_cyl(u, x) end subroutine geoflux_evaluate_cyl [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 2 __ Why: The suggestion correctly identifies and removes an empty, non-functional associate block, which is a minor code quality improvement.	Low
Update

[qodo-code-review]

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: Build and test

Failed stage: Test [❌]

Failed test name: test_geoflux_coordinate_system

Failure summary: The action failed because CTest test test_geoflux_coordinate_system (Test #28) failed and terminated with ERROR STOP 1, causing the overall test run to return a non-zero exit code (Process completed with exit code 8). - The test detected failed coordinate roundtrip checks: 5/5 roundtrip cases reported FAIL with errors on the order of ~1e-2 (e.g., err=8.42E-03, 1.51E-02, 1.62E-02), indicating the computed u_back did not match the original u within tolerance. - The failure originates from the Fortran test executable test_geoflux_coordinate_system.x in test_geoflux_coordinate_system, with the backtrace pointing to /home/runner/work/libneo/libneo/test/source/test_geoflux_coordinate_system.f90:52 (and program entry at line 3). - The Python harness /home/runner/work/libneo/libneo/test/scripts/run_geoflux_test.py then raised subprocess.CalledProcessError because the test executable exited with status 1.

Relevant error logs: 1: ##[group]Runner Image Provisioner 2: Hosted Compute Agent ... 2864: Start 21: test_vmec_modules 2865: 21/76 Test #21: test_vmec_modules ........................................ Passed 0.01 sec 2866: Start 22: test_coordinate_systems 2867: 22/76 Test #22: test_coordinate_systems .................................. Passed 0.01 sec 2868: Start 23: test_analytical_circular 2869: 23/76 Test #23: test_analytical_circular ................................. Passed 0.05 sec 2870: Start 24: test_nctools_nc_get3 2871: 24/76 Test #24: test_nctools_nc_get3 ..................................... Passed 0.01 sec 2872: Start 25: test_geoflux 2873: 25/76 Test #25: test_geoflux ............................................. Passed 2.06 sec 2874: Start 26: test_geqdsk_plot 2875: 26/76 Test #26: test_geqdsk_plot ......................................... Passed 4.12 sec 2876: Start 27: test_geoflux_plot 2877: 27/76 Test #27: test_geoflux_plot ........................................ Passed 0.73 sec 2878: Start 28: test_geoflux_coordinate_system 2879: 28/76 Test #28: test_geoflux_coordinate_system ...........................***Failed Error regular expression found in output. Regex=[STOP] 0.14 sec 2880: PASS: evaluate_cyl matches geoflux_to_cyl (18 points) 2881: PASS: evaluate_cart = cyl_to_cart(evaluate_cyl) 2882: FAIL: coordinate roundtrip at u= 0.20000000000000001 0.29999999999999999 0.0000000000000000 2883: u_back= 0.19985978324059547 0.30002700599356952 0.0000000000000000 err= 8.4222825598772033E-003 2884: FAIL: coordinate roundtrip at u= 0.50000000000000000 1.0000000000000000 0.50000000000000000 2885: u_back= 0.49959146693352835 1.0002036127133103 0.50000000000000000 err= 1.5101614064178648E-002 2886: FAIL: coordinate roundtrip at u= 0.69999999999999996 -0.50000000000000000 1.2000000000000000 2887: u_back= 0.69973651604363962 -0.50004498472337988 1.2000000000000000 err= 6.1921630796177851E-003 2888: FAIL: coordinate roundtrip at u= 0.29999999999999999 2.0000000000000000 0.80000000000000004 2889: u_back= 0.29976550839015459 1.9999455316398871 0.80000000000000004 err= 1.6248250904908446E-002 2890: FAIL: coordinate roundtrip at u= 0.90000000000000002 0.10000000000000001 0.0000000000000000 2891: u_back= 0.89985404751710596 0.10000021692946172 0.0000000000000000 err= 2.4333984277404852E-003 2892: FAIL: coordinate roundtrip 5 /5 2893: PASS: covariant_basis matches FD ( 4 points, max_err= 8.3381490120890955E-011 ) 2894: PASS: metric_tensor properties verified (3 points) 2895: FAILED: 1 error(s) in geoflux coordinate system tests 2896: ERROR STOP 1 2897: Error termination. Backtrace: 2898: #0 0x7fc4c4423e59 in ??? 2899: #1 0x7fc4c4424a71 in ??? 2900: #2 0x7fc4c442617a in ??? 2901: #3 0x557dfbff925e in test_geoflux_coordinate_system 2902: at /home/runner/work/libneo/libneo/test/source/test_geoflux_coordinate_system.f90:52 2903: #4 0x557dfbff735e in main 2904: at /home/runner/work/libneo/libneo/test/source/test_geoflux_coordinate_system.f90:3 2905: Traceback (most recent call last): 2906: File "/home/runner/work/libneo/libneo/test/scripts/run_geoflux_test.py", line 86, in <module> 2907: sys.exit(main()) 2908: ^^^^^^ 2909: File "/home/runner/work/libneo/libneo/test/scripts/run_geoflux_test.py", line 80, in main 2910: run_test(exe_path, geqdsk_path) 2911: File "/home/runner/work/libneo/libneo/test/scripts/run_geoflux_test.py", line 58, in run_test 2912: raise subprocess.CalledProcessError(result.returncode, result.args, result.returncode) 2913: subprocess.CalledProcessError: Command '['/home/runner/work/libneo/libneo/build/test/test_geoflux_coordinate_system.x', '/home/runner/work/libneo/libneo/build/test/scripts/EQDSK_I.geqdsk']' returned non-zero exit status 1. 2914: Start 29: test_arnoldi_setup ... 2995: 69/76 Test #69: tilted_coil_field_validation ............................. Passed 6.01 sec 2996: Start 70: test_polylag_5 2997: 70/76 Test #70: test_polylag_5 ........................................... Passed 0.00 sec 2998: Start 71: test_poincare 2999: 71/76 Test #71: test_poincare ............................................ Passed 0.01 sec 3000: Start 72: test_odeint_allroutines_context 3001: 72/76 Test #72: test_odeint_allroutines_context .......................... Passed 0.00 sec 3002: Start 73: test_odeint_thread_safety 3003: 73/76 Test #73: test_odeint_thread_safety ................................ Passed 0.00 sec 3004: Start 74: build_test_golden_record_odeint 3005: 74/76 Test #74: build_test_golden_record_odeint .......................... Passed 1.17 sec 3006: Start 75: test_golden_record_odeint 3007: 75/76 Test #75: test_golden_record_odeint ................................ Passed 0.21 sec 3008: Start 76: test_odeint_events 3009: 76/76 Test #76: test_odeint_events ....................................... Passed 0.00 sec 3010: 99% tests passed, 1 tests failed out of 76 3011: Label Time Summary: 3012: biot_savart = 6.58 sec*proc (1 test) 3013: build-helper = 1.17 sec*proc (1 test) 3014: field = 0.89 sec*proc (10 tests) 3015: magfie = 18.38 sec*proc (4 tests) 3016: poincare = 0.01 sec*proc (1 test) 3017: polylag = 0.00 sec*proc (1 test) 3018: tilted_coil = 11.80 sec*proc (3 tests) 3019: Total Test time (real) = 69.98 sec 3020: The following tests FAILED: 3021: Errors while running CTest 3022: 28 - test_geoflux_coordinate_system (Failed) 3023: ##[error]Process completed with exit code 8. 3024: ##[group]Run actions/upload-artifact@v4