-
Notifications
You must be signed in to change notification settings - Fork 0
Add usability improvements: Jupyter tutorials and parameter validation tool #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…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
Code Review: PR #115 - Jupyter Tutorials and Parameter ValidationSummaryThis 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. ✅ Strengths1. Excellent Code Quality
2. Well-Designed API
3. Strong Documentation
4. Physics-Informed Validation
🔍 Issues and RecommendationsCritical: Missing Test CoverageIssue: No unit tests for Impact:
Recommendation: Add 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 correctlyMinor: CLI User Experience
Minor: Code Clarity
Minor: Notebooks
Documentation: Missing References
🔬 Testing RecommendationsManual 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 resIntegration TestsAdd 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 ConsiderationsNo concerns: All validation is O(1) operations on scalar parameters. Negligible overhead. 🔒 Security Considerations
Verdict: No security issues. 📊 Additional ObservationsPositive
Questions for Author
📝 Final RecommendationApprove with minor revisions: Must fix before merge:
Should fix (can be follow-up PR):
Overall Score: 8.5/10
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
Code Review: Interactive Jupyter Notebooks & Parameter Validation ToolGreat 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: ✅ Strengths1. Excellent Documentation & User Experience
2. Robust Parameter Validation
3. Strong Test Coverage
4. Clean Code Architecture
🔍 Issues & Suggestions1. Notebook Execution Outputs Missing
|
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
Code Review - PR #115: Jupyter Tutorials and Parameter ValidationOverviewThis 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. ✅ Strengths1. Comprehensive Test Coverage (72 tests, 100% passing)
2. Clean Architecture and Code Quality
3. Excellent Documentation
4. User-Friendly CLI InterfaceThree well-designed commands: validate, suggest, check 5. Physics-Aware ValidationCorrectly implements constraints from Issue #82:
📝 Suggestions (Non-Blocking)
🔬 Code Quality Deep DiveExcellent Edge Case Handling (validation.py:198-208)
Clean Composable Designvalidate_parameters() composes smaller validation functions, making it easy to test and maintain Security
🎯 Alignment with CLAUDE.mdImplementation follows all project guidelines:
🔍 Issues Found: NONEI carefully reviewed all code changes and found:
🎉 Final Verdict: APPROVEThis PR is ready to merge. Key reasons:
Impact Assessment
Recommended Next Steps (Post-Merge)
💬 Additional CommentsThis is one of the best-reviewed PRs I've seen. The author:
Great work, @anjor! This will significantly help new users get started with GANDALF and avoid common pitfalls. Review completed by: Claude Code |
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:
Usage:
🔧 Parameter Validation Tool (
src/krmhd/validation.py)Automated parameter checking to prevent common pitfalls:
Key Validations:
CLI Interface:
Python API:
Documentation Updates
README.md Changes:
New Section: "Interactive Tutorials"
New Section: "Parameter Validation Tool"
Updated Section: "Forced Turbulence: Parameter Selection Guide"
Exports in
src/krmhd/__init__.py:Added validation functions to public API:
ValidationResultvalidate_overflow_safetyvalidate_cfl_conditionvalidate_forcing_stabilityvalidate_parametersvalidate_config_dictsuggest_parametersTesting
All features tested and verified:
Motivation
Problem 1: Learning Curve for New Users
Problem 2: Parameter Selection Pitfalls (Issue #82)
References
docs/ISSUE82_SUMMARY.md: Detailed stability analysisdocs/recommended_parameters.md: Parameter selection guideChecklist
__main__.py) with 3 commands__init__.pyFuture Work
Potential follow-ups (not in this PR):
Breaking Changes
None - this is purely additive functionality.