Skip to content

Conversation

@krystophny
Copy link
Member

@krystophny krystophny commented Dec 1, 2025

User description

Add examples/compare_ncsx_meiss_vmec_ck.py to compare guiding-center orbits in the NCSX free-boundary equilibrium using Cash–Karp GC in VMEC vs Meiss (coils) coordinates, based on the STELLOPT NCSX wout and coils files.


PR Type

Enhancement


Description

  • Add comprehensive comparison script for NCSX guiding-center orbits

  • Implements Cash–Karp RK5(4) integration in VMEC vs Meiss coordinates

  • Automates NCSX equilibrium file download and coils format conversion

  • Generates visual comparison plots of orbit trajectories (s, theta, phi)


Diagram Walkthrough

flowchart LR
  A["NCSX Files<br/>wout + coils"] -->|Download & Convert| B["Local Test Data"]
  B -->|Link| C["Work Directory"]
  C -->|Config 1| D["SIMPLE VMEC<br/>Cash–Karp"]
  C -->|Config 2| E["SIMPLE Meiss<br/>Cash–Karp"]
  D -->|orbits.nc| F["Read & Plot<br/>Comparison"]
  E -->|orbits.nc| F
  F -->|Output| G["Comparison Plot<br/>s, theta, phi vs time"]
Loading

File Walkthrough

Relevant files
Enhancement
compare_ncsx_meiss_vmec_ck.py
NCSX Cash–Karp orbit comparison and visualization script 

examples/compare_ncsx_meiss_vmec_ck.py

  • New 279-line script comparing NCSX guiding-center orbits using
    Cash–Karp RK5(4) integration
  • Implements automatic download of NCSX VMEC equilibrium and coils files
    from STELLOPT
  • Converts STELLOPT coils format to SIMPLE's coils.simple format with
    filament coordinates
  • Runs SIMPLE twice with identical parameters except field type (VMEC vs
    Meiss canonical coordinates)
  • Reads orbit trajectories from netCDF output and generates multi-panel
    comparison plots
+279/-0 

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Insecure external downloads

Description: Unauthenticated HTTP downloads of external files without integrity or certificate pinning
(via urlretrieve) allow MITM/tampering of the NCSX wout and coils inputs, which are then
executed by the simulator and could lead to compromised results or code execution if the
toolchain parses malformed files.
compare_ncsx_meiss_vmec_ck.py [62-69]

Referred Code
    print(f"Downloading NCSX wout: {NCSX_WOUT_URL}")
    urlretrieve(NCSX_WOUT_URL, wout_ncsx)

if not os.path.exists(coils_simple):
    if not os.path.exists(coils_stellopt):
        print(f"Downloading NCSX coils: {NCSX_COILS_URL}")
        urlretrieve(NCSX_COILS_URL, coils_stellopt)
Symlink race/poisoning

Description: Creation of filesystem symlinks in a shared work directory without validation can be
abused if an attacker pre-places a symlink to an arbitrary path, causing the script to
overwrite or read unintended files when linking field inputs.
compare_ncsx_meiss_vmec_ck.py [119-119]

Referred Code
os.symlink(src, dst)
Unsandboxed external execution

Description: Running an external binary (simple.x) with user-controlled configuration from the
network-fetched files without sandboxing or path validation risks arbitrary code execution
if the binary is trojaned or if crafted inputs trigger exploitable parser bugs.
compare_ncsx_meiss_vmec_ck.py [121-134]

Referred Code
print(f"Running SIMPLE [{tag}] in {run_dir}")
res = subprocess.run(
    [simple_exe, cfg_path],
    cwd=run_dir,
    capture_output=True,
    text=True,
    timeout=1800,
)
if res.returncode != 0:
    print(f"SIMPLE run '{tag}' failed")
    print("STDOUT:", res.stdout[-2000:] if len(res.stdout) > 2000 else res.stdout)
    print("STDERR:", res.stderr[-2000:] if len(res.stderr) > 2000 else res.stderr)
    raise RuntimeError(f"SIMPLE run '{tag}' failed with exit code {res.returncode}")
TOCTOU file deletion

Description: Unconditionally removing existing paths before creating symlinks (os.remove(dst) on an
attacker-controlled or unexpected path) can delete files outside the intended directory if
dst is a malicious symlink (TOCTOU).
compare_ncsx_meiss_vmec_ck.py [171-175]

Referred Code
for src, dst in ((wout_ncsx, wout_link), (coils_simple, coils_link)):
    if os.path.islink(dst) or os.path.exists(dst):
        os.remove(dst)
    os.symlink(src, dst)
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: 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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Weak error handling: Downloads, file parsing, and external process execution lack robust validation and
contextual error handling (e.g., network failures, malformed coils file, missing
variables) and sometimes exit without logging actionable context.

Referred Code
if not os.path.exists(wout_ncsx):
    print(f"Downloading NCSX wout: {NCSX_WOUT_URL}")
    urlretrieve(NCSX_WOUT_URL, wout_ncsx)

if not os.path.exists(coils_simple):
    if not os.path.exists(coils_stellopt):
        print(f"Downloading NCSX coils: {NCSX_COILS_URL}")
        urlretrieve(NCSX_COILS_URL, coils_stellopt)

    # Convert STELLOPT coils file to SIMPLE's coils.simple format
    print("Converting NCSX coils to SIMPLE format (coils.c09r00.simple)")
    coords = []
    currents = []
    in_filament = False
    with open(coils_stellopt, "r") as f:
        for line in f:
            line = line.strip()
            lower = line.lower()
            if "begin filament" in lower:
                in_filament = True
                continue


 ... (clipped 92 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing auditing: The script performs critical external actions (downloading files, executing SIMPLE, file
conversions) without emitting structured audit logs that include actor, timestamp, action,
and outcome.

Referred Code
if not os.path.exists(wout_ncsx):
    print(f"Downloading NCSX wout: {NCSX_WOUT_URL}")
    urlretrieve(NCSX_WOUT_URL, wout_ncsx)

if not os.path.exists(coils_simple):
    if not os.path.exists(coils_stellopt):
        print(f"Downloading NCSX coils: {NCSX_COILS_URL}")
        urlretrieve(NCSX_COILS_URL, coils_stellopt)

    # Convert STELLOPT coils file to SIMPLE's coils.simple format
    print("Converting NCSX coils to SIMPLE format (coils.c09r00.simple)")
    coords = []
    currents = []
    in_filament = False
    with open(coils_stellopt, "r") as f:
        for line in f:
            line = line.strip()
            lower = line.lower()
            if "begin filament" in lower:
                in_filament = True
                continue


 ... (clipped 53 lines)

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:
Verbose subprocess logs: On failure the script prints captured stdout/stderr from the external SIMPLE run to the
console, which may expose internal paths or sensitive environment details depending on
SIMPLE output.

Referred Code
if res.returncode != 0:
    print(f"SIMPLE run '{tag}' failed")
    print("STDOUT:", res.stdout[-2000:] if len(res.stdout) > 2000 else res.stdout)
    print("STDERR:", res.stderr[-2000:] if len(res.stderr) > 2000 else res.stderr)
    raise RuntimeError(f"SIMPLE run '{tag}' failed with exit code {res.returncode}")

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:
Unstructured prints: The script uses plain print statements for operational events and may echo unstructured
external process output, lacking structured logging and controls to avoid sensitive data
exposure.

Referred Code
if not os.path.exists(wout_ncsx):
    print(f"Downloading NCSX wout: {NCSX_WOUT_URL}")
    urlretrieve(NCSX_WOUT_URL, wout_ncsx)

if not os.path.exists(coils_simple):
    if not os.path.exists(coils_stellopt):
        print(f"Downloading NCSX coils: {NCSX_COILS_URL}")
        urlretrieve(NCSX_COILS_URL, coils_stellopt)

    # Convert STELLOPT coils file to SIMPLE's coils.simple format
    print("Converting NCSX coils to SIMPLE format (coils.c09r00.simple)")
    coords = []
    currents = []
    in_filament = False
    with open(coils_stellopt, "r") as f:
        for line in f:
            line = line.strip()
            lower = line.lower()
            if "begin filament" in lower:
                in_filament = True
                continue


 ... (clipped 53 lines)

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:
External input trust: The script downloads and parses external files and symlinks them without validation (e.g.,
checksums, schema/variable presence), and executes an external binary based on repository
path without verification.

Referred Code
if not os.path.exists(wout_ncsx):
    print(f"Downloading NCSX wout: {NCSX_WOUT_URL}")
    urlretrieve(NCSX_WOUT_URL, wout_ncsx)

if not os.path.exists(coils_simple):
    if not os.path.exists(coils_stellopt):
        print(f"Downloading NCSX coils: {NCSX_COILS_URL}")
        urlretrieve(NCSX_COILS_URL, coils_stellopt)

    # Convert STELLOPT coils file to SIMPLE's coils.simple format
    print("Converting NCSX coils to SIMPLE format (coils.c09r00.simple)")
    coords = []
    currents = []
    in_filament = False
    with open(coils_stellopt, "r") as f:
        for line in f:
            line = line.strip()
            lower = line.lower()
            if "begin filament" in lower:
                in_filament = True
                continue


 ... (clipped 97 lines)

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Bundle data files within repository

To improve reliability, bundle the required NCSX data files within the
repository instead of downloading them from external URLs. This change makes the
example self-contained and prevents failures from broken links.

Examples:

examples/compare_ncsx_meiss_vmec_ck.py [37-68]
NCSX_WOUT_URL = (
    "https://github.com/hiddenSymmetries/simsopt/raw/master/"
    "tests/test_files/wout_c09r00_fixedBoundary_0.5T_vacuum_ns201.nc"
)

NCSX_COILS_URL = (
    "https://princetonuniversity.github.io/STELLOPT/examples/coils.c09r00"
)



 ... (clipped 22 lines)

Solution Walkthrough:

Before:

NCSX_WOUT_URL = "https://.../wout_c09r00_fixedBoundary_0.5T_vacuum_ns201.nc"
NCSX_COILS_URL = "https://.../coils.c09r00"

def ensure_ncsx_files(test_data_dir: str):
    from urllib.request import urlretrieve
    os.makedirs(test_data_dir, exist_ok=True)

    wout_ncsx = os.path.join(test_data_dir, "wout_ncsx.nc")
    if not os.path.exists(wout_ncsx):
        print(f"Downloading NCSX wout: {NCSX_WOUT_URL}")
        urlretrieve(NCSX_WOUT_URL, wout_ncsx)

    # ... similar logic for coils file
    # ...
    return wout_ncsx, coils_simple

After:

def ensure_ncsx_files(test_data_dir: str):
    # Data files are now part of the repository, no download needed.
    # The function would just verify existence and perform the conversion.
    os.makedirs(test_data_dir, exist_ok=True)

    wout_ncsx = os.path.join(test_data_dir, "wout_ncsx.nc")
    coils_stellopt = os.path.join(test_data_dir, "coils.c09r00")
    coils_simple = os.path.join(test_data_dir, "coils.c09r00.simple")

    if not os.path.exists(coils_simple):
        # Convert the bundled STELLOPT coils file
        print("Converting NCSX coils to SIMPLE format...")
        # ... conversion logic ...

    return wout_ncsx, coils_simple
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a significant reliability issue with downloading files from external URLs, which could break the example in the future, and proposes a robust solution to make it self-contained.

Medium
Possible issue
Prevent crashes when creating symlinks

Improve symlink creation by handling cases where the destination path is an
existing directory. Use os.path.lexists() and shutil.rmtree() to prevent
potential OSError exceptions.

examples/compare_ncsx_meiss_vmec_ck.py [171-174]

 for src, dst in ((wout_ncsx, wout_link), (coils_simple, coils_link)):
-    if os.path.islink(dst) or os.path.exists(dst):
-        os.remove(dst)
+    if os.path.lexists(dst):
+        if os.path.isdir(dst) and not os.path.islink(dst):
+            import shutil
+            shutil.rmtree(dst)
+        else:
+            os.remove(dst)
     os.symlink(src, dst)
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that os.remove() will fail on a directory and provides a robust solution using os.path.lexists() and shutil.rmtree() to handle this edge case, improving the script's reliability.

Low
  • More

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants