Skip to content

Conversation

@anjor
Copy link
Owner

@anjor anjor commented Nov 24, 2025

Overview

This PR adds interactive Jupyter notebooks and automated parameter validation to improve user experience and help avoid common numerical instabilities.

What's New

🎓 Interactive Jupyter Tutorials (notebooks/)

Three comprehensive tutorial notebooks for hands-on learning:

Notebook Topic Runtime Key Concepts
01_getting_started.ipynb First simulation (~10 min) ~2s Grid setup, forcing, energy monitoring
02_driven_turbulence.ipynb Forced turbulence (~30 min) ~20s Energy spectra, k⁻⁵/³ scaling, parameter scans
03_analyzing_decay.ipynb Decaying turbulence (~20 min) ~1-2 min Exponential decay, selective decay, spectral slopes

Usage:

uv run jupyter notebook notebooks/01_getting_started.ipynb

🔧 Parameter Validation Tool (src/krmhd/validation.py)

Automated parameter checking to prevent common pitfalls:

Key Validations:

CLI Interface:

# Validate config file
python -m krmhd validate configs/driven_turbulence.yaml

# Get parameter suggestions for 64³ forced turbulence
python -m krmhd suggest --resolution 64 64 32 --type forced

# Quick parameter check
python -m krmhd check --dt 0.01 --eta 0.5 --nu 0.5 --Nx 64 --Ny 64 --Nz 32

Python API:

from krmhd import validate_parameters, suggest_parameters

# Validate parameters
result = validate_parameters(
    dt=0.01, eta=0.5, nu=0.5,
    Nx=64, Ny=64, Nz=32,
    force_amplitude=0.3
)

if not result.valid:
    result.print_report()  # Shows errors and suggestions

# Get automated suggestions
params = suggest_parameters(Nx=64, Ny=64, Nz=32, simulation_type="forced")

Documentation Updates

README.md Changes:

  1. New Section: "Interactive Tutorials"

    • Overview of all three notebooks
    • Usage instructions
    • Tips for getting started
  2. New Section: "Parameter Validation Tool"

    • CLI command examples
    • Python API documentation
    • List of checks performed
    • Cross-references to detailed docs
  3. Updated Section: "Forced Turbulence: Parameter Selection Guide"

    • Added prominent link to validation tool
    • Directs users to automated checking

Exports in src/krmhd/__init__.py:

Added validation functions to public API:

  • ValidationResult
  • validate_overflow_safety
  • validate_cfl_condition
  • validate_forcing_stability
  • validate_parameters
  • validate_config_dict
  • suggest_parameters

Testing

All features tested and verified:

# ✅ CLI commands work
python -m krmhd suggest --resolution 64 64 32 --type forced
python -m krmhd check --dt 0.01 --eta 0.5 --nu 0.5 --Nx 64 --Ny 64 --Nz 32
python -m krmhd validate configs/driven_turbulence.yaml

# ✅ Python imports work
python -c "from krmhd import validate_parameters, suggest_parameters"

# ✅ Notebooks have valid JSON structure
python -c "import json; json.load(open('notebooks/01_getting_started.ipynb'))"

Motivation

Problem 1: Learning Curve for New Users

  • Previous state: Only Python script examples
  • New solution: Interactive Jupyter notebooks with explanations
  • Benefit: Faster onboarding, encourages experimentation

Problem 2: Parameter Selection Pitfalls (Issue #82)

  • Previous state: Manual parameter selection, easy to hit instabilities
  • New solution: Automated validation with clear error messages
  • Benefit: Prevents common mistakes, saves debugging time

References

Checklist

  • Three Jupyter notebooks created and tested
  • Parameter validation module implemented
  • CLI interface (__main__.py) with 3 commands
  • Python API exported via __init__.py
  • README updated with new sections
  • All features tested (CLI, Python API, notebooks)
  • Commit message follows convention

Future Work

Potential follow-ups (not in this PR):

  • CI workflow for testing notebooks (nbconvert + pytest)
  • Additional notebooks for advanced topics (Hermite moments, field line following)
  • Interactive parameter tuning widget for notebooks
  • Sphinx documentation website with notebook galleries

Breaking Changes

None - this is purely additive functionality.

…n tool

This commit adds interactive Jupyter notebooks and automated parameter
validation to improve user experience and help avoid common pitfalls.

## Jupyter Tutorial Notebooks (notebooks/)

Three interactive tutorials covering key concepts:

1. **01_getting_started.ipynb** (~10 min)
   - First GANDALF simulation
   - Setting up grids and initial conditions
   - Running forced turbulence
   - Energy monitoring and visualization
   - Runtime: ~2 seconds

2. **02_driven_turbulence.ipynb** (~30 min)
   - Comprehensive forced turbulence with diagnostics
   - Energy spectra: E(k), E(k⊥), E(k∥)
   - Inertial range scaling (k⁻⁵/³)
   - Energy balance analysis
   - Parameter scanning
   - Runtime: ~20 seconds

3. **03_analyzing_decay.ipynb** (~20 min)
   - Decaying turbulence analysis
   - Exponential energy decay fitting
   - Selective decay demonstration
   - Spectral evolution
   - Runtime: ~1-2 minutes

All notebooks encourage hands-on experimentation and include extensive
explanations of the physics.

## Parameter Validation Module (src/krmhd/validation.py)

Automated parameter checking to avoid numerical instabilities:

### Key Features:
- **Overflow safety checks**: η·dt < 50, ν·dt < 50 (Issue #82)
- **CFL condition validation**: dt < CFL_limit × dx / v_A
- **Forcing stability checks**: Energy injection vs dissipation balance
- **Resolution-dependent suggestions**: Auto-suggest safe parameters

### CLI Interface (src/krmhd/__main__.py):
```bash
# Validate config file
python -m krmhd validate configs/driven_turbulence.yaml

# Get parameter suggestions
python -m krmhd suggest --resolution 64 64 32 --type forced

# Quick parameter check
python -m krmhd check --dt 0.01 --eta 0.5 --nu 0.5 --Nx 64 --Ny 64 --Nz 32
```

### Python API:
```python
from krmhd import validate_parameters, suggest_parameters

result = validate_parameters(dt=0.01, eta=0.5, nu=0.5, Nx=64, Ny=64, Nz=32)
if not result.valid:
    result.print_report()

params = suggest_parameters(Nx=64, Ny=64, Nz=32, simulation_type="forced")
```

## Documentation Updates (README.md)

Added two new sections:

1. **Interactive Tutorials** - Guide to using Jupyter notebooks
2. **Parameter Validation Tool** - CLI and Python API documentation

Also added cross-reference in "Forced Turbulence: Parameter Selection
Guide" section directing users to automated validation.

## Testing

- ✅ All CLI commands tested and working
- ✅ Python imports verified
- ✅ Notebook JSON structure validated
- ✅ Config file validation tested

References:
- docs/ISSUE82_SUMMARY.md - Forced turbulence stability
- docs/recommended_parameters.md - Parameter selection guide
- CLAUDE.md - Hyper-dissipation implementation
@claude
Copy link

claude bot commented Nov 24, 2025

Code Review: PR #115 - Jupyter Tutorials and Parameter Validation

Summary

This PR adds excellent usability improvements to GANDALF with three Jupyter notebooks and a comprehensive parameter validation module. The additions are well-designed, well-documented, and directly address user pain points (learning curve and parameter selection). Overall, this is a strong PR that significantly improves the user experience.

✅ Strengths

1. Excellent Code Quality

  • Clean architecture: Validation logic separated into focused functions with clear responsibilities
  • Type hints: Full type annotations in validation.py and __main__.py
  • Documentation: Comprehensive docstrings with parameter descriptions and references
  • Functional style: Pure functions returning structured results (matches project conventions in CLAUDE.md)

2. Well-Designed API

  • ValidationResult dataclass: Clean separation of errors, warnings, suggestions
  • Multiple entry points: CLI (python -m krmhd), Python API (validate_parameters()), config validation
  • Composable functions: validate_overflow_safety(), validate_cfl_condition(), validate_forcing_stability() can be used individually
  • Good defaults: Sensible thresholds (50.0 for overflow, 1.0 for CFL)

3. Strong Documentation

4. Physics-Informed Validation

  • Overflow safety: Correctly implements normalized dissipation constraints from CLAUDE.md
  • CFL condition: Standard numerical stability check with safety factors
  • Forcing stability: Addresses Issue Investigate 64³ resolution instability anomaly in Alfvénic cascade #82 findings (amplitude vs resolution, injection/dissipation balance)
  • Resolution-dependent suggestions: Smart parameter scaling in suggest_parameters()

🔍 Issues and Recommendations

Critical: Missing Test Coverage

Issue: No unit tests for validation.py or integration tests for __main__.py CLI.

Impact:

  • No verification that validation logic is correct
  • Risk of regressions in future changes
  • Can't verify edge cases (e.g., extreme parameters, missing config keys)

Recommendation: Add tests/test_validation.py with:

def test_overflow_safety_errors():
    # Should fail for η·dt ≥ 50
    result = validate_overflow_safety(eta=60.0, dt=1.0)
    assert not result.valid
    assert any('overflow' in e.lower() for e in result.errors)

def test_overflow_safety_warnings():
    # Should warn for η·dt > 40 (0.8 * 50)
    result = validate_overflow_safety(eta=45.0, dt=1.0)
    assert result.valid  # Not an error
    assert len(result.warnings) > 0

def test_cfl_violation():
    # dt * v_A / dx > 1.0 should fail
    result = validate_cfl_condition(dt=0.1, dx=0.05, v_A=1.0)
    assert not result.valid

def test_suggest_parameters_forced():
    params = suggest_parameters(64, 64, 32, simulation_type="forced")
    # Verify overflow safety
    assert params['eta'] * params['dt'] < 50
    assert params['nu'] * params['dt'] < 50
    # Verify CFL
    dx_min = 1.0 / 64
    cfl = params['dt'] * params['v_A'] / dx_min
    assert cfl < 1.0

def test_validate_config_dict_nested():
    # Test nested config structure
    config = {
        "grid": {"Nx": 64, "Ny": 64, "Nz": 32},
        "physics": {"hyper_r": 2},
        "forcing": {"amplitude": 0.3, "n_max": 2}
    }
    result = validate_config_dict(config)
    # Should handle nested structure correctly

Minor: CLI User Experience

  1. Missing help text in validation output

    • validation.py:113: When suggesting η < threshold/dt, provide context (e.g., "For 50× safety margin")
    • validation.py:203: CFL suggestion doesn't explain why 0.3-0.5 is better than 1.0
  2. No exit code documentation

    • __main__.py:28,50,128: Document return codes (0 = success, 1 = failure) in docstrings
  3. Suggestion command always validates

    • __main__.py:78-96: After suggesting parameters, it validates them (always passes). Consider removing redundant validation or make it optional with --validate flag.

Minor: Code Clarity

  1. Magic numbers without constants

    • validation.py:114,132: 0.8 * threshold appears twice. Define WARNING_THRESHOLD_RATIO = 0.8 at module level.
    • validation.py:277: 10 * dissipation_estimate - magic number. Define INJECTION_DISSIPATION_RATIO_THRESHOLD = 10.
  2. Incomplete type hints

    • __main__.py:22,53,101: Functions return int but not annotated. Add -> int:
  3. Potential division by zero

    • validation.py:112,130: If dt=0 or eta=0, division in suggestions will fail. Add guard:
    if dt > 0 and eta > 0:
        suggestions.append(f"Reduce η to < {threshold/dt:.4f} or reduce dt to < {threshold/eta:.4f}")

Minor: Notebooks

  1. No execution outputs saved

    • Notebooks have empty outputs: [] fields. Consider running notebooks and saving outputs so users can preview results on GitHub without executing.
  2. Hardcoded paths

    • 01_getting_started.ipynb: No explicit instructions for running from different directories. Add note about working directory.
  3. No error handling examples

    • Consider adding a cell in tutorial 01 demonstrating what happens with invalid parameters (educational)

Documentation: Missing References

  1. validation.py:17: References docs/ISSUE82_SUMMARY.md and docs/recommended_parameters.md but PR doesn't verify these exist.

  2. README.md:892-894: Lists three docs that may not exist in repo. Verify with:

    ls docs/ISSUE82_SUMMARY.md docs/recommended_parameters.md

🔬 Testing Recommendations

Manual Testing Checklist

# ✅ Test CLI commands work
python -m krmhd suggest --resolution 64 64 32 --type forced
python -m krmhd check --dt 0.01 --eta 0.5 --nu 0.5 --Nx 64 --Ny 64 --Nz 32
python -m krmhd validate configs/driven_turbulence.yaml  # ⚠️  Verify config exists!

# ✅ Test Python imports work
python -c "from krmhd import validate_parameters, suggest_parameters"

# ✅ Test notebooks have valid JSON
python -c "import json; json.load(open('notebooks/01_getting_started.ipynb'))"

# ✅ Test edge cases
python -m krmhd check --dt 0.0001 --eta 1000 --nu 1000 --Nx 32 --Ny 32 --Nz 16  # Should fail
python -m krmhd suggest --resolution 256 256 128 --type forced  # High res

Integration Tests

Add tests/test_cli.py using subprocess.run() to test CLI interface:

import subprocess

def test_cli_suggest():
    result = subprocess.run(
        ['python', '-m', 'krmhd', 'suggest', '--resolution', '64', '64', '32', '--type', 'forced'],
        capture_output=True, text=True
    )
    assert result.returncode == 0
    assert 'dt:' in result.stdout

🚀 Performance Considerations

No concerns: All validation is O(1) operations on scalar parameters. Negligible overhead.

🔒 Security Considerations

  1. YAML loading: __main__.py:35 uses yaml.safe_load() ✅ (safe, prevents code execution)
  2. File paths: __main__.py:24 validates file existence before opening ✅
  3. No user input injection: CLI args parsed with argparse type checking ✅

Verdict: No security issues.

📊 Additional Observations

Positive

  • Consistent style: Matches existing codebase conventions (JAX, functional, type-hinted)
  • Non-breaking: Purely additive, no changes to existing APIs
  • Export management: __init__.py properly exports new functions to public API
  • Future-proof: Validation functions accept hyper_r, hyper_n for future extensions

Questions for Author

  1. Have you tested validate configs/driven_turbulence.yaml on all 7 config files in configs/?
  2. Do the referenced docs (ISSUE82_SUMMARY.md, recommended_parameters.md) exist?
  3. Any plans for Jupyter CI testing (e.g., pytest --nbval)?

📝 Final Recommendation

Approve with minor revisions:

Must fix before merge:

  • ❗ Add unit tests for validation.py (at minimum: test overflow safety, CFL, forcing stability)
  • ❗ Verify referenced documentation files exist

Should fix (can be follow-up PR):

  • Add CLI integration tests
  • Add constants for magic numbers
  • Save notebook execution outputs for preview
  • Add type hints to CLI functions

Overall Score: 8.5/10

  • Excellent design and implementation
  • Critical gap in test coverage
  • Outstanding documentation and user experience

Great work on this PR! The validation tool will save users significant debugging time, and the notebooks lower the barrier to entry considerably. With test coverage added, this will be a very solid addition to GANDALF.

Let me know if you'd like me to help write the tests! 🧪

…ments

This commit addresses all feedback from PR #115 code review, implementing
comprehensive testing, code quality improvements, and better documentation.

## Test Coverage Added

### Unit Tests (tests/test_validation.py) - 46 tests
Comprehensive coverage of all validation functions:
- ValidationResult dataclass and print_report()
- validate_overflow_safety() - 13 tests including edge cases
- validate_cfl_condition() - 6 tests including division-by-zero
- validate_forcing_stability() - 6 tests
- validate_parameters() - comprehensive integration (6 tests)
- suggest_parameters() - 6 tests including validation of suggestions
- validate_config_dict() - 4 tests for flat and nested configs
- Edge cases - 5 tests for extreme/invalid inputs

**Key test scenarios:**
- Overflow detection (η·dt ≥ 50, ν·dt ≥ 50)
- Near-threshold warnings (> 80% of limits)
- CFL violations and warnings
- Forcing amplitude vs resolution constraints
- Energy injection/dissipation balance
- Zero/negative parameter handling
- Division-by-zero safety

### CLI Integration Tests (tests/test_cli_validation.py) - 26 tests
Complete CLI command coverage:
- validate command: valid/invalid configs, missing files (4 tests)
- suggest command: all simulation types, custom CFL (7 tests)
- check command: valid/invalid parameters, optional args (5 tests)
- Help system: all commands, main help (5 tests)
- Output formatting: separators, checkmarks (3 tests)
- Error handling: malformed YAML, invalid inputs (2 tests)

**All 72 tests passing** ✅

## Code Quality Improvements

### 1. Constants for Magic Numbers (validation.py)
Replaced hardcoded values with named constants:
```python
OVERFLOW_SAFETY_THRESHOLD = 50.0
WARNING_THRESHOLD_RATIO = 0.8
INJECTION_DISSIPATION_RATIO_LIMIT = 10.0
DEFAULT_CFL_LIMIT = 1.0
CFL_WARNING_RATIO = 0.8
HIGH_RES_AMPLITUDE_LIMIT_128 = 0.4
HIGH_RES_AMPLITUDE_LIMIT_256 = 0.3
```

**Benefits:**
- Self-documenting code
- Single source of truth
- Easy to adjust thresholds

### 2. Type Hints Added (__main__.py)
All CLI functions now have complete type annotations:
- cmd_validate(args: argparse.Namespace) -> int
- cmd_suggest(args: argparse.Namespace) -> int
- cmd_check(args: argparse.Namespace) -> int
- main() -> int

**Benefits:**
- Better IDE support
- Clearer function contracts
- Type checking with mypy

### 3. Division-by-Zero Handling (validation.py)
Added safety check in validate_cfl_condition():
```python
if dx <= 0:
    errors.append(f"Invalid grid spacing: dx = {dx} (must be > 0)")
    return ValidationResult(False, warnings_list, errors, suggestions)
```

**Prevents:** RuntimeError when dx=0, gracefully returns error message

### 4. Enhanced CLI Help Text (__main__.py)
Expanded epilog with:
- Validation checks explained with thresholds
- Resolution-specific amplitude limits
- Exit code documentation (0=success, 1=failure)
- Links to detailed documentation files

**User benefit:** Self-documenting CLI with all key info in --help

## Documentation

### Notebook README (notebooks/README.md)
Comprehensive guide covering:
- Installation and setup instructions
- Overview of all 3 tutorials with runtimes
- Recommended learning path
- Tips for experimentation
- Troubleshooting common issues
- Exporting figures and notebooks

**Addresses:** Working directory clarification from feedback

### Verified Documentation References
Confirmed all referenced files exist:
- ✅ docs/ISSUE82_SUMMARY.md (9.5 KB)
- ✅ docs/recommended_parameters.md (9.2 KB)
- ✅ CLAUDE.md (13 KB)

## Testing

All tests pass on Python 3.13:
```bash
$ uv run pytest tests/test_validation.py tests/test_cli_validation.py -v
========================= 72 passed in 16.47s =========================

# Unit tests: 46 passed
# CLI integration tests: 26 passed
```

### Test Coverage Summary
- **Validation logic:** 100% of public functions tested
- **CLI commands:** All 3 commands tested (validate, suggest, check)
- **Edge cases:** Division-by-zero, invalid inputs, extreme values
- **Integration:** End-to-end CLI workflows via subprocess

## Changes Summary

**Added:**
- tests/test_validation.py (46 tests, 439 lines)
- tests/test_cli_validation.py (26 tests, 293 lines)
- notebooks/README.md (200+ lines of documentation)
- 7 validation constants in validation.py
- Type hints for all CLI functions
- Division-by-zero safety check

**Modified:**
- src/krmhd/validation.py: Constants, edge case handling
- src/krmhd/__main__.py: Type hints, enhanced help text

**Testing:**
- Total: 732 lines of test code
- Coverage: All validation functions, all CLI commands
- Pass rate: 100% (72/72 tests)

## Addresses PR Feedback

✅ **Critical:** Add unit tests for validation.py
✅ **Critical:** Add integration tests for CLI
✅ Define constants for magic numbers (0.8, 50, etc.)
✅ Add type hints to CLI functions
✅ Handle division-by-zero edge cases
✅ Verify referenced documentation files exist
✅ Add help text explaining validation thresholds
✅ Document CLI exit codes
✅ Clarify working directory requirements (notebooks README)

**Note on notebook outputs:** Skipped saving execution outputs to avoid
large diffs in version control. Users can run notebooks interactively to
see outputs. notebooks/README.md provides complete usage instructions.

## References

- PR #115 review comment
- docs/ISSUE82_SUMMARY.md - Forced turbulence stability
- docs/recommended_parameters.md - Parameter selection
- PEP 484 - Type Hints
- PEP 257 - Docstring Conventions
@claude
Copy link

claude bot commented Nov 24, 2025

Code Review: Interactive Jupyter Notebooks & Parameter Validation Tool

Great work on adding these usability improvements! This PR significantly enhances the user experience with comprehensive tutorials and automated parameter checking. Here's my detailed review:


✅ Strengths

1. Excellent Documentation & User Experience

  • The three-tier notebook progression (Getting Started → Driven Turbulence → Analyzing Decay) is pedagogically sound
  • Clear learning objectives and runtime estimates help users set expectations
  • The CLI interface is well-designed with intuitive commands (validate, suggest, check)

2. Robust Parameter Validation

  • Overflow safety checks are mathematically sound and match the normalized dissipation formulation in CLAUDE.md
  • CFL validation properly accounts for grid spacing and velocity
  • Forcing stability checks address Issue Investigate 64³ resolution instability anomaly in Alfvénic cascade #82 concerns effectively
  • Resolution-dependent warnings (128³, 256³) are practical and evidence-based

3. Strong Test Coverage

  • Comprehensive unit tests for all validation functions (test_validation.py: 464 lines)
  • CLI integration tests ensure end-to-end functionality (test_cli_validation.py: 343 lines)
  • Good edge case coverage (overflow, warnings, invalid inputs)

4. Clean Code Architecture

  • Functional decomposition: each validation concern is a separate function
  • ValidationResult dataclass provides clean interface
  • CLI commands properly separated into cmd_validate, cmd_suggest, cmd_check
  • Proper use of type hints throughout

🔍 Issues & Suggestions

1. Notebook Execution Outputs Missing ⚠️

Severity: Medium

All three notebooks have "execution_count": null and empty outputs. While this keeps the PR diff clean, it reduces their value as reference documentation.

Recommendation:

  • Execute all notebooks once and commit with outputs
  • This helps users verify expected behavior without running them first
  • Consider adding a pre-commit hook to strip outputs in future PRs if desired

Example verification:

# Execute and save outputs
jupyter nbconvert --to notebook --execute --inplace notebooks/*.ipynb

2. Parameter Validation Thresholds Could Use References 🔧

Severity: Low

The magic numbers in validation.py are reasonable but lack citations:

OVERFLOW_SAFETY_THRESHOLD = 50.0  # Where does this come from?
INJECTION_DISSIPATION_RATIO_LIMIT = 10.0  # Empirical or theoretical?
HIGH_RES_AMPLITUDE_LIMIT_128 = 0.4  # From Issue #82, but not linked

Recommendation:

# Example improved documentation
OVERFLOW_SAFETY_THRESHOLD = 50.0  
"""Maximum safe value for η·dt (prevents exp(50) overflow).
Based on float64 max exponent ≈ 709, with safety factor.
Reference: CLAUDE.md "Normalized Dissipation" section.
"""

HIGH_RES_AMPLITUDE_LIMIT_128 = 0.4
"""Empirical stability limit from Issue #82 investigation.
128³ runs with amplitude > 0.4 exhibited exponential growth.
"""

3. CLI Help Text: Missing Newlines 🔧

Severity: Low

The --help epilog in __main__.py:176-203 is well-written but the formatting could be improved:

Current:

epilog="""
Examples:
  # Validate a config file
  python -m krmhd validate configs/driven_turbulence.yaml
..."""

Suggestion: Consider using dedent for cleaner code:

from textwrap import dedent

epilog=dedent("""\
    Examples:
      # Validate a config file
      python -m krmhd validate configs/driven_turbulence.yaml
    ...""")

4. Potential Division by Zero in CFL Validation ⚠️

Severity: Low (handled, but could be clearer)

In validate_cfl_condition (line 198-202), there's a check for dx <= 0, which is good. However, the function also has v_A parameter that could theoretically be zero or negative.

Current code:

cfl_actual = dt * v_A / dx  # What if v_A <= 0?

Recommendation:

if v_A <= 0:
    errors.append(f"Invalid Alfvén velocity: v_A = {v_A} (must be > 0)")
    return ValidationResult(False, warnings_list, errors, suggestions)

5. Forcing Stability Heuristic Could Be More Precise 🔧

Severity: Low

The forcing stability check (line 292-302) uses a rough estimate:

dissipation_estimate = eta * k_force**2
injection_estimate = force_amplitude**2  # Simplified

This is a reasonable heuristic, but the actual injection rate depends on the forcing implementation (force_alfven_modes). Consider adding a comment noting this is a lower bound check.

Suggestion:

# Rough heuristic: actual injection depends on forcing implementation
# This provides a conservative lower bound for stability
dissipation_estimate = eta * k_force**2
injection_estimate = force_amplitude**2  # ε_inj ∝ amplitude² (approx)

6. Notebooks: Hardcoded Kernel Version 🔧

Severity: Low

All notebooks specify:

"version": "3.10.0"

This might cause compatibility warnings for users on Python 3.11+. Consider:

  1. Using `"version": "3"" (language version only)
  2. Or updating to a more recent/flexible version specifier

7. Missing Import in Validation Module ⚠️

Severity: Medium

validation.py uses warnings module (imported line 22) but never actually uses it. Either:

  • Remove the unused import, or
  • Consider using warnings.warn() for runtime warnings instead of just returning ValidationResult warnings

If keeping the import, consider this pattern:

if eta_dt > WARNING_THRESHOLD_RATIO * threshold:
    msg = f"Resistivity near overflow: η·dt = {eta_dt:.2f}"
    warnings.warn(msg, UserWarning)
    warnings_list.append(msg)

8. Test Coverage: Config Dict Edge Cases 🔧

Severity: Low

validate_config_dict (line 486-530) handles nested dicts well:

Nx = config.get("Nx", config.get("grid", {}).get("Nx", 64))

But tests don't verify this nested behavior. Add a test like:

def test_validate_nested_config():
    config = {
        "dt": 0.01,
        "grid": {"Nx": 64, "Ny": 64, "Nz": 32},
        "physics": {"eta": 0.02, "nu": 0.02},
        "forcing": {"amplitude": 0.3, "n_max": 2}
    }
    result = validate_config_dict(config)
    assert result.valid

9. README Updates: Link Validation Tool Earlier 🔧

Severity: Low

The new "Parameter Validation Tool" section is great (line 819), but users see "Forced Turbulence: Parameter Selection Guide" first (line 973). Consider:

  1. Moving the validation tool section earlier, or
  2. Adding a bold callout at the top of the forced turbulence section

Example:

## Forced Turbulence: Parameter Selection Guide

> 🔧 **NEW:** Use `python -m krmhd validate` to automatically check parameters! See [Parameter Validation Tool](#parameter-validation-tool).

**CRITICAL for forced turbulence simulations**: ...

10. Notebooks: Consider Adding Runtime Estimates 🔧

Severity: Low

The notebooks mention runtime in the overview, but it would be helpful to add per-section estimates for longer notebooks (02, 03). Example:

## 3. Run Forced Turbulence Simulation

⏱️ **Estimated runtime:** ~15 seconds (200 steps on 64³ grid)

The simulation loop:
...

🧪 Testing Suggestions

Missing Tests:

  1. CLI exit codes: Verify validate returns 0 on success, 1 on failure
  2. YAML parsing errors: What happens with malformed YAML?
  3. Suggest command validation: Currently generates but doesn't validate the suggested params
  4. Notebook JSON validation: Could add a simple test to ensure notebooks are valid JSON

Example test:

def test_suggest_command_generates_valid_params():
    result = run_cli("suggest", "--resolution", "64", "64", "32")
    assert result.returncode == 0
    # Check that suggested params would pass validation
    assert "✓" in result.stdout or "valid" in result.stdout.lower()

🔒 Security Considerations

All clear - No security concerns identified:

  • YAML loading uses yaml.safe_load() (not load()) ✓
  • No user input is executed directly ✓
  • Tempfile handling in tests is proper ✓
  • No SQL/command injection vectors ✓

📊 Performance Considerations

Efficient implementation:

  • JAX imports are lazy (only when needed)
  • Validation functions are pure Python (fast)
  • No unnecessary array allocations
  • CLI has minimal startup overhead

Minor note: validate_forcing_stability computes k_force**2 using JAX (jnp.pi). This is fine but could use plain math.pi for consistency with other validation functions.


📝 Documentation Quality

Strengths:

Minor improvements:

  • Add a "See Also" section to validation.py docstring linking to CLI commands
  • Consider adding examples to function docstrings for validate_parameters

🎯 Overall Assessment

Quality: High ⭐⭐⭐⭐⭐
Test Coverage: Good ⭐⭐⭐⭐
Documentation: Excellent ⭐⭐⭐⭐⭐
Code Style: Consistent ⭐⭐⭐⭐⭐

Recommendation: APPROVE with minor suggestions

This is a high-quality PR that significantly improves usability. The parameter validation tool directly addresses Issue #82 concerns, and the notebooks provide an excellent onboarding experience.

Before merging:

  1. ✅ Execute notebooks and commit outputs (recommended)
  2. ✅ Add validation for v_A <= 0 in CFL check (recommended)
  3. ⚠️ Remove unused warnings import (optional)
  4. ⚠️ Add test for nested config dict validation (optional)

Post-merge enhancements:

  • Consider CI integration for notebook testing (pytest --nbval)
  • Add pre-commit hooks for notebook output stripping
  • Create a "parameter cookbook" doc with recommended configs for common scenarios

🙏 Great work!

This PR represents a significant step forward in making GANDALF more accessible to new users while preventing common pitfalls. The combination of interactive tutorials and automated validation is exactly what the project needs.

Estimated review time: 45 minutes
Files reviewed: 10/10
Tests verified: ✅ (structure confirmed, execution recommended before merge)

Addresses final PR feedback by adding validation for non-positive
Alfvén velocity in validate_cfl_condition().

Changes:
- Add v_A <= 0 check before computing CFL number
- Prevents potential division issues with invalid velocities
- Returns clear error message for invalid v_A values

Tests Added:
- test_zero_v_A_edge_case: Validates v_A=0 handling
- test_negative_v_A_edge_case: Validates v_A<0 handling

All 74 tests passing (72 previous + 2 new).

Addresses: PR #115 follow-up review feedback
@claude
Copy link

claude bot commented Nov 24, 2025

Code Review - PR #115: Jupyter Tutorials and Parameter Validation

Overview

This is an excellent PR that significantly enhances the user experience of GANDALF. The addition of interactive Jupyter notebooks and automated parameter validation addresses two critical pain points.


✅ Strengths

1. Comprehensive Test Coverage (72 tests, 100% passing)

  • Unit tests (test_validation.py): 46 tests
  • CLI integration tests (test_cli_validation.py): 26 tests
  • Edge cases: Division-by-zero, negative values handled gracefully
  • Test quality: Well-organized with clear docstrings

2. Clean Architecture and Code Quality

  • Named constants replace magic numbers
  • Type hints on all public functions
  • Dataclass validation result with clean reporting
  • Functional style throughout
  • DRY principle applied

3. Excellent Documentation

  • CLI help text includes all validation thresholds
  • Notebook README (148 lines) covers installation/troubleshooting
  • Docstrings follow numpy style with references
  • README updates integrate new features clearly

4. User-Friendly CLI Interface

Three well-designed commands: validate, suggest, check

5. Physics-Aware Validation

Correctly implements constraints from Issue #82:

  • Overflow safety: η·dt < 50, ν·dt < 50
  • CFL condition: dt < CFL_limit × dx / v_A
  • Forcing stability: Energy injection vs dissipation balance
  • Resolution constraints: Amplitude limits for 128³ (0.4) and 256³ (0.3)

📝 Suggestions (Non-Blocking)

  1. Notebook Outputs: Consider adding CI job to test notebooks execute successfully
  2. Config File Support: Consider TOML/JSON support in future PR
  3. Documentation: validate_forcing_stability() always returns valid=True - worth documenting explicitly

🔬 Code Quality Deep Dive

Excellent Edge Case Handling (validation.py:198-208)

  • Checks for dx <= 0 before CFL calculation
  • Checks for v_A <= 0 with clear error messages
  • Returns graceful error messages instead of crashes

Clean Composable Design

validate_parameters() composes smaller validation functions, making it easy to test and maintain

Security

  • ✅ Uses yaml.safe_load() to prevent arbitrary code execution
  • ✅ Uses pathlib.Path for cross-platform file handling
  • ✅ CLI arguments properly validated by argparse

🎯 Alignment with CLAUDE.md

Implementation follows all project guidelines:

  1. ✅ Functional style - Pure functions throughout
  2. ✅ Explicit typing - Full type hints
  3. ✅ JAX patterns - Proper use of jnp
  4. ✅ Physics transparency - Variable names match physics notation
  5. ✅ Documentation - Comprehensive docstrings with references

🔍 Issues Found: NONE

I carefully reviewed all code changes and found:

  • ✅ No bugs
  • ✅ No security concerns
  • ✅ No performance issues
  • ✅ No breaking changes
  • ✅ No missing edge cases

🎉 Final Verdict: APPROVE

This PR is ready to merge. Key reasons:

  1. High-quality implementation with proper error handling
  2. Excellent documentation
  3. Addresses real user needs
  4. Thorough testing - 72 tests with 100% pass rate
  5. Follows project conventions perfectly
  6. No breaking changes

Impact Assessment

  • User experience: 📈 Significant improvement
  • Code quality: 📈 High standards maintained
  • Test coverage: 📈 Excellent coverage added
  • Documentation: 📈 Comprehensive and clear
  • Maintainability: 📈 Well-structured and extensible

Recommended Next Steps (Post-Merge)

  1. Consider adding CI workflow for notebook testing
  2. Consider TOML/JSON config support in future PR
  3. Monitor Issue Investigate 64³ resolution instability anomaly in Alfvénic cascade #82 for additional forcing stability constraints

💬 Additional Comments

This is one of the best-reviewed PRs I've seen. The author:

  • Addressed every piece of feedback from initial review
  • Added comprehensive tests (72 tests!)
  • Improved code quality (constants, type hints, edge case handling)
  • Enhanced documentation (notebooks README, CLI help text)

Great work, @anjor! This will significantly help new users get started with GANDALF and avoid common pitfalls.


Review completed by: Claude Code
Review date: 2025-11-24
Lines reviewed: 3,432 additions across 10 files
Test coverage: 732 lines of test code, 72 tests passing

@anjor anjor merged commit 3ed1f89 into main Nov 24, 2025
1 check passed
@anjor anjor deleted the feature/usability-improvements branch November 24, 2025 20:48
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.

2 participants