feat/fitting#6
Merged
Merged
Conversation
Owner
Ameyanagi
commented
Feb 10, 2026
Ameyanagi
commented
- Add FEFF85L fitting APIs with Rust runner and uv fixture workflow
- Redesign FEFF fitting API and add xraylarch comparison outputs
- feat(fitting): add FEFF batch parallel API and benchmark
- refactor(fitting): simplify to joint/independent APIs with explicit batch strategy
There was a problem hiding this comment.
Pull request overview
This PR adds a new Rust-core FEFF85L fitting subsystem to xraytsubaki, including expression-driven parameterization, a consuming-self builder API, multi-dataset (joint) fitting, batch-parallel fitting utilities, and documentation/fixtures/scripts for parity comparisons against xraylarch.
Changes:
- Introduces
xafs::fittingmodule with FEFF85L path parsing (feffNNNN.dat),path2chi/ff2chi, expression evaluation + symbol extraction, transforms, and LM-based solver with joint + independent batch execution. - Adds a new ergonomic builder API (
FeffFit,FeffFitDataset,Param) and re-exports viaprelude, plus examples/benchmarks for batch fitting and larch comparison exports. - Adds xraylarch-generated fixtures and plot-generation scripts/workflow assets to validate/visualize parity.
Reviewed changes
Copilot reviewed 43 out of 100 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| py-xraytsubaki/src/lib.rs | Adds fitting error categorization for Python bindings. |
| plans/fitting-api-redesign.md | Design plan for the new builder API, expression engine, and multi-dataset fitting. |
| doc/scripts/generate_feff_vs_larch_png.py | New script to render PNG comparisons from CSV parity outputs. |
| doc/scripts/generate_feff_vs_larch_plots.py | New script to generate SVG plots + an index from CSV parity outputs. |
| doc/plots/feff_vs_larch_index.md | Generated index of comparison plot outputs. |
| crates/xraytsubaki/tests/testfiles/feffcu01.dat | Adds FEFF fixture path file for tests/parity. |
| crates/xraytsubaki/tests/testfiles/feff_path_chi_larch_ref.txt | Adds larch reference output for path2chi parity. |
| crates/xraytsubaki/tests/testfiles/feff_fit_target_larch.txt | Adds larch reference target curve for fitting parity. |
| crates/xraytsubaki/tests/testfiles/feff_ff2chi_larch_ref.txt | Adds larch reference output for ff2chi parity. |
| crates/xraytsubaki/tests/testfiles/feff0002.dat | Adds second FEFF fixture path file for multi-path tests/parity. |
| crates/xraytsubaki/tests/pythonscript/pyproject.toml | Adds uv-managed Python project for regenerating larch fixtures. |
| crates/xraytsubaki/tests/pythonscript/generate_test.py | Extends fixture generator to produce FEFF fitting parity references. |
| crates/xraytsubaki/tests/pythonscript/README.md | Documents how to regenerate fixtures via uv. |
| crates/xraytsubaki/tests/pythonscript/.gitignore | Ignores uv venv and Python cache. |
| crates/xraytsubaki/tests/feff_builder_examples.rs | Adds integration-style tests demonstrating builder workflows and larch parity checks. |
| crates/xraytsubaki/src/xafs/mod.rs | Registers xafs::fitting and wires FittingError into the top-level XAFSError. |
| crates/xraytsubaki/src/xafs/fitting/variables.rs | Adds variable graph resolution + expression symbol extraction wrappers. |
| crates/xraytsubaki/src/xafs/fitting/transform.rs | Adds R-space transform application, residual construction, and n_idp computation. |
| crates/xraytsubaki/src/xafs/fitting/solver.rs | Implements joint solver + independent batch execution and associated tests. |
| crates/xraytsubaki/src/xafs/fitting/path_model.rs | Implements FEFF path modeling (path2chi/ff2chi) and interpolation utilities. |
| crates/xraytsubaki/src/xafs/fitting/mod.rs | Registers fitting submodules and provides public API façade functions/re-exports. |
| crates/xraytsubaki/src/xafs/fitting/feffdat.rs | Implements FEFF85L feffNNNN.dat parsing + tests. |
| crates/xraytsubaki/src/xafs/fitting/expression.rs | Adds pest-based expression parsing/evaluation + function/constant support + caching. |
| crates/xraytsubaki/src/xafs/fitting/expression.pest | Defines the expression grammar (operators, calls, constants, identifiers). |
| crates/xraytsubaki/src/xafs/fitting/errors.rs | Introduces FittingError enum for fitting/runner/expression/parser errors. |
| crates/xraytsubaki/src/xafs/fitting/builder.rs | Adds FeffFit consuming builder, auto-discovery, and builder tests. |
| crates/xraytsubaki/src/prelude.rs | Re-exports fitting APIs/types to the crate prelude. |
| crates/xraytsubaki/examples/feff_compare_export.rs | Adds an example exporting parity CSVs for plotting and comparisons. |
| crates/xraytsubaki/doc/feff-fitting-mvp.md | Documents MVP scope, boundaries, and migration guidance for fitting. |
| crates/xraytsubaki/benches/feff_fitting_batch_benchmark.rs | Adds criterion benchmark for batch independent fitting performance. |
| crates/xraytsubaki/README.md | Updates project README with FEFF fitting MVP feature bullets and scope notes. |
| crates/xraytsubaki/Cargo.toml | Adds pest_derive dependency and registers new benchmark target. |
| Cargo.toml | Adds pest_derive to workspace dependencies. |
| Cargo.lock | Lockfile updates for new dependencies pulled in by expression parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile OverviewGreptile SummaryOverviewThis PR introduces a new FEFF fitting surface area, including a Rust-based FEFF runner, a redesigned fitting API (joint/independent), batch-parallel evaluation support with a benchmark, and fixtures/scripts for comparing outputs against xraylarch. How it fits the codebaseThe bulk of the change lands under Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| Cargo.lock | Adds pest_derive and hashing/crypto transitive deps to lockfile; no issues found. |
| Cargo.toml | Adds pest_derive to workspace dependencies; no issues found. |
| crates/xraytsubaki/Cargo.toml | Adds pest_derive dependency and registers new fitting batch benchmark; no issues found. |
| crates/xraytsubaki/src/xafs/fitting/builder.rs | Adds FEFF fitting builder API supporting joint/independent and batch strategies; no issues found. |
| crates/xraytsubaki/src/xafs/fitting/expression.pest | Adds pest grammar for expression parsing; no issues found (minor latent ambiguity for identifier 'e' not exercised). |
| crates/xraytsubaki/src/xafs/fitting/runner.rs | Added FEFF85L runner integration (exec, discovery, timeout); no issues found. |
| crates/xraytsubaki/src/xafs/fitting/solver.rs | Added LM solver with joint/independent modes and batch strategies; no issues found. |
| crates/xraytsubaki/src/xafs/fitting/transform.rs | Added FFT transform/windowing and residual computations for fitting; no issues found. |
| crates/xraytsubaki/src/xafs/fitting/types.rs | Added core fitting types (datasets, strategies, results, serde); no issues found. |
| crates/xraytsubaki/tests/feff_builder_examples.rs | Added integration tests covering builder, transforms, fitting workflows; no issues found. |
| py-xraytsubaki/src/lib.rs | Exposed fitting module through Python bindings; no issues found. |
Sequence Diagram
sequenceDiagram
participant User as Caller
participant Builder as FittingBuilder
participant Model as PathModel/FitProblem
participant Vars as FitVariables
participant Runner as FeffRunner
participant Solver as Solver (joint/independent)
User->>Builder: configure dataset(s), paths, variables, transform
Builder->>Model: build FitProblem (datasets + models)
Model->>Vars: compile expressions / parameter mapping
alt needs FEFF data
Model->>Runner: run FEFF85L (batch/parallel)
Runner-->>Model: feff.dat / chi(k) outputs
end
User->>Solver: solve(problem, strategy)
Solver->>Model: evaluate residuals (k/R space)
Model->>Vars: apply parameter vector (clamp bounds)
Model-->>Solver: residuals + jacobian/metrics
Solver-->>User: FitResult (best params, stats, exports)
Greptile OverviewGreptile SummaryThis PR introduces a new FEFF fitting subsystem under
Key issues to address before merge are concentrated in the modeling/solver edge-cases: the interpolation helper can panic when fitting explores parameter regions that produce Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| crates/xraytsubaki/src/xafs/fitting/solver.rs | Implements joint/independent FEFF fitting with LM solver; issues: interp_linear_clamped panic risk via ff2chi (see path_model), per-dataset DOF uses global n_vary, Jacobian step is unscaled (numerical stability). |
| crates/xraytsubaki/src/xafs/fitting/path_model.rs | Adds FEFF path modeling (feffpath/path2chi/ff2chi) and interpolation; bug: partial_cmp(...).unwrap() can panic if computed q contains NaN (e.g., during fitting when parameters produce invalid energies). |
| crates/xraytsubaki/src/xafs/fitting/variables.rs | Adds variable resolution and expression evaluation; bug: resolve_path_param silently replaces non-finite expression results with default values, hiding errors and corrupting fits. |
| crates/xraytsubaki/src/xafs/fitting/transform.rs | Implements R-space transform and residual calculation with masking; appears consistent, no merge-blocking issues found. |
| crates/xraytsubaki/src/xafs/fitting/runner.rs | Implements FEFF85L module runner with command resolution, staging, logs, and tests using mock executables; no merge-blocking issues found. |
| crates/xraytsubaki/src/xafs/fitting/types.rs | Introduces core data types for FEFF fitting/run/batch options and result structs; no merge-blocking issues found. |
| crates/xraytsubaki/src/xafs/fitting/feffdat.rs | Adds FEFF85L feffNNNN.dat parser with validation and tests; no merge-blocking issues found. |
| crates/xraytsubaki/src/xafs/fitting/mod.rs | Wires fitting module exports and public API wrappers to xafs::Result/XAFSError; no merge-blocking issues found. |
Sequence Diagram
sequenceDiagram
autonumber
actor Caller
participant API as xafs::fitting (mod.rs)
participant Solver as solver::feffit_joint
participant Vars as FitVariables/expressions
participant Model as path_model::ff2chi
participant FFT as transform::apply_r_transform
participant LM as LevenbergMarquardt
Caller->>API: feffit_joint(datasets, vars)
API->>Solver: feffit_joint(...)
Solver->>Solver: validate_dataset (k/chi, paths, transform)
Solver->>FFT: apply_r_transform(data chi)
Solver->>LM: minimize(problem)
loop iterations
LM->>Solver: set_params(params)
Solver->>Vars: apply_parameter_vector
Solver->>Model: ff2chi(paths, vars, k)
Model->>Vars: resolve_values / resolve_path_param
Model-->>Solver: model chi + per-path chi
Solver->>FFT: apply_r_transform(model chi)
Solver-->>LM: residuals + jacobian
end
LM-->>Solver: solved problem
Solver->>Model: ff2chi(paths, solved vars, k)
Solver->>FFT: apply_r_transform(model + path chis)
Solver-->>API: FeffFitResult (metrics + datasets)
API-->>Caller: Result<FeffFitResult>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile OverviewGreptile SummaryThis PR introduces a new FEFF85L fitting stack in The core data flow is: parse FEFF Must-fix before merge: the FEFF Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| crates/xraytsubaki/src/xafs/fitting/feffdat.rs | Implements FEFF85L .dat parser; current header detection uses unsafe UTF-8 string slicing that can panic on non-ASCII input. |
| crates/xraytsubaki/src/xafs/fitting/path_model.rs | Adds FEFF path-to-chi computation and interpolation; interpolation uses partial_cmp(...).unwrap() which can panic on NaN inputs (previously reported) and should be guarded. |
| crates/xraytsubaki/src/xafs/fitting/solver.rs | Introduces joint/independent Levenberg–Marquardt solver and batch strategies; main flow looks consistent, though per-dataset DOF and Jacobian step sizing concerns exist (already discussed in prior threads). |
| crates/xraytsubaki/src/xafs/fitting/transform.rs | Adds R-space transform and residual computation with input validation; residual masking logic is guarded and errors on empty residual. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Solver as solver::feffit_joint
participant Problem as FeffFitMultiProblem
participant PathModel as path_model::ff2chi
participant Transform as transform::apply_r_transform
participant LM as LevenbergMarquardt
Caller->>Solver: "feffit_joint(datasets, variables)"
Solver->>Solver: "validate_dataset() for each"
Solver->>Problem: "FeffFitMultiProblem::new(datasets, variables, varying_names)"
loop "init data transforms"
Problem->>Transform: "apply_r_transform(k, data_chi, transform)"
Transform-->>Problem: "TransformOutput (data)"
end
Solver->>LM: "minimize(problem)"
loop "LM iterations"
LM->>Problem: "set_params(x)"
LM->>Problem: "residuals() / jacobian()"
Problem->>PathModel: "ff2chi(paths, vars, k)"
PathModel-->>Problem: "model chi + per-path chi"
Problem->>Transform: "apply_r_transform(k, model_chi, transform)"
Transform-->>Problem: "TransformOutput (model)"
Problem-->>LM: "residual vector"
end
LM-->>Solver: "solved problem + report"
Solver->>Problem: "evaluate_models(solved.vars)"
loop "per dataset output"
Solver->>Transform: "residual_in_r_space(data_transform, model_transform)"
end
Solver-->>Caller: "FeffFitResult"
Greptile OverviewGreptile SummaryThis PR introduces a new FEFF85L fitting stack: a FEFF runner that resolves and executes module binaries, a FEFF Overall, the core modeling/solver flow is coherent: datasets are validated, variables are resolved/applied, models are generated via Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| crates/xraytsubaki/src/xafs/fitting/path_model.rs | Adds FEFF path modeling APIs (feffpath/path2chi/ff2chi), parameter resolution, and linear interpolation with input validation + tests. |
| crates/xraytsubaki/src/xafs/fitting/transform.rs | Implements R-space transform pipeline, residual computation, N_idp calculation, and transform validation with unit tests. |
| crates/xraytsubaki/examples/feff_compare_export.rs | New example exporting FEFF vs Larch comparison outputs; large but self-contained. |
| crates/xraytsubaki/src/xafs/fitting/builder.rs | Introduces/updates fluent builders for datasets/paths/variables; integrates with solver types. |
| crates/xraytsubaki/src/xafs/fitting/errors.rs | Defines fitting/runner error types used across modules; consistent error mapping. |
| crates/xraytsubaki/src/xafs/fitting/expression.rs | Implements expression parsing/evaluation and symbol extraction; used by variables/path params. |
| crates/xraytsubaki/src/xafs/fitting/feffdat.rs | Implements FEFF85L .dat parser with monotonic/finite validation; includes string slicing on title/header lines but likely safe for ASCII FEFF output. |
| crates/xraytsubaki/src/xafs/fitting/runner.rs | Adds FEFF module runner, command resolution, workspace staging, and path discovery with tests. |
| crates/xraytsubaki/src/xafs/fitting/solver.rs | Adds LM-based joint and independent batch solver, per-dataset diagnostics, and covariance stderr computation. |
| crates/xraytsubaki/src/xafs/fitting/types.rs | Defines core fitting/runner data types and defaults used across modules. |
| crates/xraytsubaki/src/xafs/fitting/variables.rs | Implements variable resolution (including expression dependencies) and parameter vector mapping for solver. |
| crates/xraytsubaki/tests/feff_builder_examples.rs | Adds integration-style tests demonstrating builder usage and end-to-end fitting. |
Sequence Diagram
sequenceDiagram
participant Client
participant Solver as feffit_joint (solver.rs)
participant Vars as FitVariables (variables.rs)
participant Model as ff2chi/path2chi (path_model.rs)
participant Xform as apply_r_transform (transform.rs)
participant LM as LevenbergMarquardt
Client->>Solver: feffit_joint(datasets, variables)
Solver->>Solver: validate_dataset() + validate_transform()
Solver->>Vars: varying_names()
Solver->>LM: minimize(FeffFitMultiProblem)
loop per iteration
LM->>Solver: residuals()/jacobian()
Solver->>Vars: apply_parameter_vector()
Solver->>Model: ff2chi(paths, vars, k)
Model->>Vars: resolve_values() + resolve_params()
Model-->>Solver: model chi + per-path chi
Solver->>Xform: apply_r_transform(k, chi, transform)
Xform-->>Solver: TransformOutput
Solver->>Solver: residual_in_r_space(data, model, epsilon_k)
Solver-->>LM: residual vector
end
LM-->>Solver: solved problem
Solver-->>Client: FeffFitResult (variables + datasets)
Greptile OverviewGreptile SummaryThis PR adds a new The new APIs are wired into the crate’s public surface via One blocking issue was found in the FEFF runner: it waits for module exit before draining stdout/stderr, which can deadlock on pipe backpressure for verbose modules. Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| crates/xraytsubaki/src/xafs/fitting/builder.rs | Adds builder-style FeffFit API with dataset defaults and auto-discovery of variables from path expressions; no blocking issues found. |
| crates/xraytsubaki/src/xafs/fitting/runner.rs | Adds Rust FEFF runner that stages feff.inp, resolves sibling/PATH modules, executes them with optional timeout, and collects feffNNNN.dat outputs; contains a likely deadlock risk when reading stdout/stderr after wait. |
| crates/xraytsubaki/src/xafs/fitting/solver.rs | Adds LM-based joint solver plus independent batch runner, per-dataset diagnostics, and Jacobian/covariance estimation; overall sound but see runner-style residual penalty handling and potential silent penalties. |
| crates/xraytsubaki/src/xafs/fitting/expression.rs | Implements cached pest-based expression parser/evaluator with symbol extraction and finite-result enforcement; no definite issues found. |
| crates/xraytsubaki/src/xafs/fitting/path_model.rs | Implements FEFF path modeling (ff2chi/path2chi), parameter resolution, and safe interpolation with finite checks; previous NaN/UTF-8 panic paths appear addressed. |
| crates/xraytsubaki/src/xafs/fitting/feffdat.rs | Adds FEFF85L .dat parser with UTF-8-safe splitting and monotonic/finite validation; no definite issues found. |
| crates/xraytsubaki/src/xafs/fitting/transform.rs | Adds R-space transform utilities and residual computation with masking and configuration validation; no definite issues found. |
| crates/xraytsubaki/src/xafs/fitting/types.rs | Adds core fitting types (datasets, variables, params, results, batch options, runner request/result) used across the new fitting module; no definite issues found. |
| crates/xraytsubaki/src/xafs/fitting/variables.rs | Adds FitVariables parameter vector mapping, expression-graph resolution with cycle detection, and strict non-finite rejection for resolved path params; no definite issues found. |
| crates/xraytsubaki/src/xafs/fitting/mod.rs | Wires the new fitting module and re-exports APIs; no definite issues found. |
| crates/xraytsubaki/src/xafs/mod.rs | Exposes fitting module and integrates FittingError into the top-level XAFSError enum; no definite issues found. |
Sequence Diagram
sequenceDiagram
autonumber
participant U as User/Caller
participant B as FeffFit (builder)
participant S as solver::feffit_joint
participant P as path_model::ff2chi
participant T as transform::apply_r_transform
participant LM as LevenbergMarquardt
U->>B: FeffFit::new()
U->>B: data()/add_path()/set_inits()/...
U->>B: fit()
B->>B: collect_default_dataset()
B->>B: auto_discover_variables()
B->>S: feffit_joint(datasets, vars)
S->>S: validate_dataset() for each dataset
S->>T: apply_r_transform(data chi)
S->>LM: minimize(FeffFitMultiProblem)
loop iterations
LM->>S: residuals()/jacobian()
S->>P: ff2chi(paths, vars, k)
P-->>S: model chi + per-path chi
S->>T: apply_r_transform(model chi)
S-->>LM: residual vector
end
S-->>B: FeffFitResult + per-dataset diagnostics
B-->>U: FeffFitResult (with warnings)
Comment on lines
+100
to
+103
| (Ok(result), Ok(())) => Ok(result), | ||
| (Err(run_err), Ok(())) => Err(run_err), | ||
| (Ok(_), Err(restore_err)) => Err(restore_err), | ||
| (Err(run_err), Err(_restore_err)) => Err(run_err), |
There was a problem hiding this comment.
Child process I/O deadlock
run_single_module waits for the child to exit (wait_for_exit) before draining stdout/stderr. If a FEFF module writes enough output to fill the pipe buffer, the child can block on write and never exit, and the parent will spin until timeout (or forever if no timeout), even though the process is still running. This is reproducible when a module is chatty.
Fix by reading stdout/stderr concurrently while the child runs (e.g., spawn threads to read_to_end and then wait, or use wait_with_output).
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/xraytsubaki/src/xafs/fitting/runner.rs
Line: 100:103
Comment:
**Child process I/O deadlock**
`run_single_module` waits for the child to exit (`wait_for_exit`) *before* draining `stdout`/`stderr`. If a FEFF module writes enough output to fill the pipe buffer, the child can block on write and never exit, and the parent will spin until timeout (or forever if no timeout), even though the process is still running. This is reproducible when a module is chatty.
Fix by reading stdout/stderr concurrently while the child runs (e.g., spawn threads to `read_to_end` and then `wait`, or use `wait_with_output`).
How can I resolve this? If you propose a fix, please make it concise.
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
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.