Physics sharpness and code conciseness audit - #4
Merged
Merged
Conversation
get_coupling() previously fabricated the electronic coupling from the diabatic energies (1e-6 placeholder at the crossing, 0.5*|E1-E2| away from it), which is not physically meaningful, and referenced E1/E2 before assignment on the auto-crossing path (NameError). H_ab must now be supplied from an electronic-structure calculation, via the constructor or get_coupling(H_ab=...); get_coupling still locates and stores the diabatic crossing. Marcus lambda, activation energy, rate, and mobility are unchanged. Unskip the transfer-coordinate test module (22 tests had never run) and fix latent errors in the tests themselves: reorganization-energy bounds now use the exact harmonic result lambda = a*dQ^2, and the downhill-transfer case passes delta_G = -0.3 instead of +0.3.
Capture coefficients from ConfigCoordinate assume a neutral defect. Add sommerfeld_parameter(temperature, Z, m_eff, eps0, method) in analysis/sommerfeld.py to correct for Coulomb enhancement/suppression: C_charged(T) = s(T) * C(T). Two methods: Maxwell-Boltzmann thermal average of the exact Coulomb factor (default), and the analytic low-temperature limits of Passler, phys. stat. sol. (b) 78, 625 (1976). Follows the CarrierCapture.jl / nonrad convention (Z < 0 attractive, Z > 0 repulsive, Z = 0 gives 1). Adds electron rest energy, fine-structure constant, and Rydberg energy to _constants.py, exports from carriercapture and carriercapture.analysis, tests, and a user-guide section.
The committed benchmark report showed overall_passed: false (capture coefficient 1.5% off Julia vs 1% tolerance; final eigenvalues 2e-4 vs 1e-4), while the README claimed all comparisons passed and attributed the gaps to floating-point differences. Root cause, verified to machine precision: CarrierCapture.jl's solve1D_Quantum builds its finite-difference kinetic term with grid spacing dq = (Q_max-Q_min)/N, while its range(Qi, Qf, length=N) grid actually has spacing (Q_max-Q_min)/(N-1), and it integrates overlaps with the rectangle rule. Re-running Python with those conventions emulated reproduces the Julia eigenvalues to ~3e-12 and C(300K) to ~5e-12. Python's native numerics (true spacing, trapezoid rule) are closer to the analytic harmonic eigenvalues. The benchmark now reports three tiers: (1) algorithmic equivalence with Julia conventions emulated (binding, rtol 1e-9/1e-6), (2) native eigenvalues vs analytic E0 + hw*(n+1/2) (binding, rtol 5e-4), and (3) native C(300K) vs Julia (informational, rtol 2e-2, with the root cause recorded in the report). README validation tables updated to match; benchmark_report.json regenerated with overall_passed: true.
The electron-phonon coupling matrix element W was labeled 'eV' in code docstrings, CLI help, docs, README, AGENT.md, and benchmark comments. Dimensional analysis of C = V*(2pi/hbar)*g*W^2*|S|^2*delta with the overlap S in amu^0.5*Ang and delta in 1/eV requires W in eV/(amu^0.5*Ang) for C in cm^3/s, matching the CarrierCapture.jl Wif convention. The formula itself was already correct; only the unit labels were wrong. Also clean up schrodinger.py: replace the muddled kinetic-term derivation comment with the correct one-line unit statement, fix docstring examples that passed a bare 0.5*hw*Q^2 potential (which does not give hbar*omega*(n+1/2) eigenvalues), and correct the normalization docstring (rectangle rule, not trapezoid).
parameter_scan: - Remove the fabricated W coupling formula (hardcoded 0.068 eV magic constant). ScanParameters now takes an explicit W (required by ParameterScanner); the scan CLI gains a required -W/--coupling option and the dashboard scan tab gains a W input. - Failed crossing searches record NaN instead of a phantom 50 eV barrier, and only catch ValueError/RuntimeError. - ScanResult.save() now persists ScanParameters (JSON payload in npz/hdf5), so save/load round-trips instead of losing the scan configuration. - Replace the three divergent joblib code paths (one of which passed an invalid 'delayed(...) + tuple' expression to Parallel) with a single helper. doped_interface: - Fix estimate_phonon_frequency conversion: hw = (hbar*c/sqrt(AMU))* sqrt(k) = 0.0647*sqrt(k), not the previous 0.0041357*sqrt(k) (h in eV*ps, wrong constant) which returned phonon energies 15.6x too small. Add an exact round-trip regression test. - Remove a dead callable() conditional and unused variables in calculate_Q0_crossing; document that get_available_charge_states returns only the entry's own charge state. visualization: - Add public plot_scan_heatmap (heatmap/contour/both), referenced by the docs but previously nonexistent; the dashboard scan figure and the scan-plot CLI now both route through it. - Consolidate five hand-written plotly layout blocks in static.py into shared _LAYOUT_DEFAULTS. io/CLI: - Single detect_format() helper replaces three copies of the extension-to-format dict. - capture CLI uses None-sentinel defaults for degeneracy/cutoff/sigma so explicit values equal to the default are no longer silently overridden by config-file values. - capture --plot now reuses visualization.plot_capture_coefficient instead of a second matplotlib reimplementation.
- mkdocs.yml: remove ~13 dangling nav entries (Theory, Tutorials, Development sections and changelog were deleted earlier but the nav was never updated); remove stale docs/TEST_RESULTS.md and the empty development/ directory. - Fix documented APIs that do not exist: estimate_phonon_energy -> estimate_phonon_frequency (io.doped_interface), load_potential -> load_potential_from_file, save_results/read_csv_data -> write_capture_results/read_csv, cc.volume / cc.Q0 attribute snippets (volume and Q0 are method arguments, not stored attributes). - AGENT.md: replace the nonexistent scan_parameters() example with the real ParameterScanner/ScanParameters API, fix the g= kwarg (degeneracy=), correct the test-file list, and update the benchmark section to the three-tier results. - README/docs: correct test count (169), remove nonexistent [viz] and [all] install extras, link all four tutorial notebooks, align Python version claims with the 3.9-3.12 classifiers, and mark the PyPI install as coming soon in docs/index.md. Note: 'Error reading page api/analysis.md' from mkdocstrings pre-exists on main and is unchanged here.
Docs call plot_scan_heatmap with colorscale/width/height kwargs; accept a colorscale parameter and pass layout overrides through. Fix doc snippets that read temperature from ScanResult directly (it lives on ScanResult.parameters).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full audit of the package for physics correctness and code conciseness. Core solver math (constants, kinetic term, overlaps, delta normalization, capture prefactor, Boltzmann occupation) verified correct against CODATA 2014 and analytic results. Seven commits:
Physics fixes
C = V·(2π/ℏ)·g·W²·|S|²·δrequires eV/(amu^0.5·Å) (CarrierCapture.jl Wif convention). Formula was already correct — only the labels were wrong.overall_passed: false(C(300K) 1.5% off Julia) while the README claimed PASS and blamed floating point. Verified to machine precision: the entire gap comes from CarrierCapture.jl'ssolve1D_Quantumusing finite-difference spacingΔQ/Nwhile its grid has true spacingΔQ/(N−1), plus rectangle-rule overlaps. Emulating those conventions reproduces Julia's eigenvalues to ~3e-12 and C(300K) to ~5e-12. The benchmark now runs three tiers (algorithmic equivalence / native-vs-analytic / native-vs-Julia informational) and passes honestly. Worth reporting the δq convention upstream to CarrierCapture.jl.estimate_phonon_frequencyreturned ℏω 15.6× too small: conversion factor was 0.0041357 (Planck constant in eV·ps, wrong constant) instead ofℏc/√AMU = 0.06465. Fixed with exact round-trip regression test.1e-6placeholder /0.5·|E1−E2|). H_ab must now be supplied from an electronic-structure calculation. Unskipped the 22-test module (never ran; fixed latent physics errors in the tests themselves).sommerfeld_parameter(T, Z, m_eff, eps0, method)for charged-defect capture (exact MB thermal average + Pässler 1976 analytic limits), with tests and a user-guide section.W = 0.068/(dQ−Q_m)formula; W is now a required, explicit scan parameter (API, CLI-W, dashboard input).Conciseness / correctness cleanups
ScanResult.save()/load()now round-trips the scan parameters (previously lost).delayed(...) + tupletoParallel) with a single helper; failed crossings record NaN instead of a phantom 50 eV barrier.plot_scan_heatmapand routed the dashboard scan figure andscan-plotCLI through it; consolidated five duplicated plotly layout blocks; singledetect_format()helper replaces three copies of the extension map; capture CLI--plotreuses the plotly Arrhenius plot instead of a matplotlib reimplementation.-W 0,--Q0 0,--cutoff,--sigma,-gvalues are no longer silently overridden by config files.doped_interface; muddled derivation comments inschrodinger.pyreplaced with correct one-liners; misleading docstring examples fixed.Docs/metadata
TEST_RESULTS.md; fixed hallucinated APIs in docs (estimate_phonon_energy,load_potential,save_results,cc.volume,cc.Q0,scan_parameters(),g=kwarg); corrected test count (169), removed nonexistent[viz]/[all]extras, linked all four notebooks, aligned Python version claims.Note:
mkdocs builderror onapi/analysis.md(mkdocstrings) pre-exists on main and is unchanged here.Test plan
pytest tests/: 138 passed, 31 skipped (doped optional) locally; 169 collected.python benchmarks/benchmark_sn_zn.py: exit 0, Tier-1 diffs ≤ 5e-12.capture/scan/scan-plot --help, import sweep of all changed public symbols.🤖 Generated with Claude Code