Port RF parameter fitting

[bernalde]

Summary

• Add RF KBB fitting transforms for Kvwf, Bf, and Bvw, including log-space and bounded logistic variants.
• Extend shared primary-drying fitting helpers to solve either PikalParams or RFParams while preserving PrimaryDryFit, err_expT, and obj_expT behavior.
• Add explicit RF fitting entry points: gen_sol_rf, err_rf, obj_rf, and fit_rf_primary_drying.
• Export the new RF fitting API and update the Julia parity matrix.

Closes #46

Tests

• python -m ruff check lyopronto/fitting.py lyopronto/__init__.py tests/test_rf.py
• PYTHONPATH=/tmp/LyoPRONTO-issue-46 MPLCONFIGDIR=/tmp/mpl pytest --rootdir=/tmp/lyopronto_issue_46_tests -c /dev/null /tmp/lyopronto_issue_46_tests/test_rf.py -q -n 0 -> 10 passed
• PYTHONPATH=/tmp/LyoPRONTO-issue-46 MPLCONFIGDIR=/tmp/mpl pytest --rootdir=/tmp/lyopronto_issue_46_tests -c /dev/null /tmp/lyopronto_issue_46_tests/test_primary_drying_fit.py /tmp/lyopronto_issue_46_tests/test_fitting.py -q -n 0 -> 11 passed
• git diff --check

Notes

• Focused pytest files were copied to /tmp/lyopronto_issue_46_tests for local execution because this /tmp/LyoPRONTO-issue-46 worktree path causes pytest to collect the repository root __init__.py as a top-level module.

[bernalde]

Blocking issues:

• method="minimize" is part of the new RF public API and issue #46's acceptance criteria require synthetic scalar-optimizer recovery, but this PR only tests that path with a monkeypatched optimizer that returns the exact answer. That leaves the real scipy.optimize.minimize RF path unverified. I would not merge this until the blocking issue above is addressed.

Nonblocking issues:

• None.

Questions:

• None.

Tests run and outcomes:

• Confirmed PR diff totals against origin/main...origin/fix/issue-46-rf-parameter-fitting: 4 files, +439/-33, matching gh pr view and gh pr diff.
• python -m ruff check lyopronto/fitting.py lyopronto/__init__.py tests/test_rf.py -> passed.
• PYTHONPATH=/tmp/LyoPRONTO-issue-46 MPLCONFIGDIR=/tmp/mpl pytest --rootdir=/tmp/lyopronto_pr60_review_tests -c /dev/null /tmp/lyopronto_pr60_review_tests/tests/test_rf.py -q -n 0 -> 10 passed.
• PYTHONPATH=/tmp/LyoPRONTO-issue-46 MPLCONFIGDIR=/tmp/mpl pytest --rootdir=/tmp/lyopronto_pr60_review_tests -c /dev/null /tmp/lyopronto_pr60_review_tests/tests/test_primary_drying_fit.py /tmp/lyopronto_pr60_review_tests/tests/test_fitting.py -q -n 0 -> 11 passed.
• PYTHONPATH=/tmp/LyoPRONTO-issue-46 MPLCONFIGDIR=/tmp/mpl pytest --rootdir=/tmp/lyopronto_pr60_review_tests -c /dev/null /tmp/lyopronto_pr60_review_tests/tests -n auto -v -m "not notebook and not pyomo" -> 299 passed, 2 skipped, 1 setup-artifact failure because the external root initially lacked examples/example_optimizer.py; after copying examples/, the failing test passed in isolation.
• Live PR checks via gh pr checks 60 --repo SECQUOIA/LyoPRONTO: SciPy Tests passed, doctests passed, Deploy docs passed, optional Pyomo Tests skipped.

Merge-readiness:

• Not ready until the RF method="minimize" path is covered by a real optimizer test or removed from the claimed scope. Because this review is being posted from the PR author's account, I am submitting it as COMMENT; formal approval/request-changes must come from another maintainer.

[bernalde]

Blocking: this test does not exercise the real RF method="minimize" path. fit_rf_primary_drying exposes that mode and issue #46 requires synthetic scalar-optimizer recovery, but this monkeypatch returns the exact theta regardless of whether SciPy can minimize the RF objective. Please add a real minimize recovery test (possibly a smaller deterministic case or a slow test) that verifies Kvwf, Bf, and Bvw within the issue tolerances, or narrow the API/PR scope so this mode is not claimed yet.

[bernalde]

Addressed in 4a493d0. The fake optimizer was removed and tests/test_rf.py::test_fit_rf_primary_drying_minimize_recovers_kbb now runs real SciPy Nelder-Mead, checks success and real evaluations, verifies objective improvement, and confirms Kvwf, Bf, and Bvw are within the issue tolerances.

[bernalde]

Senior maintainer review of #60 (Port RF parameter fitting).

Note on event type: I am the PR author, so GitHub rejects a formal APPROVE / REQUEST_CHANGES from me (HTTP 422); submitting as COMMENT. The formal verdict must be recorded by another maintainer. Bottom line: no blocking issues; the change is merge-ready on the merits, with one nonblocking item and one question.

1. Blocking issues

None.

2. Nonblocking issues

• RF minimize-path recovery is verified only via a monkeypatched minimize, not end to end. test_fit_rf_primary_drying_minimize_attaches_rf_result replaces scipy.optimize.minimize with a fake that returns the exact solution, so it checks the wiring (method forwarded, theta0 zeros, fit_method/fitted_params attached) but not that an actual minimize run recovers Kvwf/Bf/Bvw. The least-squares path is recovered end to end (test_fit_rf_primary_drying_least_squares_recovers_kbb) and obj_rf is shown to be 0 at the exact point, so the substance is covered; but issue #46's first acceptance criterion references the Julia minimize-based recovery (test/test_RF_opt.jl#L61). Consider one small end-to-end minimize recovery test (a cheap optimizer/low maxiter is fine) or note explicitly that least-squares covers recovery while minimize covers wiring. See inline comment.

3. Questions

• fit_rf_primary_drying accepts only a single RFParams + single PrimaryDryFit, whereas the generic fit_primary_drying it delegates to (and gen_nsol_pd/objn_pd) support multi-experiment sequences, and the parity matrix now describes gen_nsol_pd as a "Multi-experiment Pikal/RF fitting helper." Is single-experiment-only the intended RF convenience surface (multi-experiment RF fitting being reached via fit_primary_drying directly)? If so, a one-line docstring note would set expectations. See inline comment.

4. Tests run and outcomes

Clean worktree at PR head (fd8892a), isolated venv, pip install -e .[dev]:

• ruff check lyopronto/fitting.py lyopronto/__init__.py tests/test_rf.py -> All checks passed
• MPLCONFIGDIR=/tmp/mpl pytest tests/test_rf.py tests/test_primary_drying_fit.py tests/test_fitting.py tests/test_pikal.py -n 0 -> 29 passed (RF fitting plus the conventional fitting suites, confirming conventional behavior is unchanged)
• pytest tests/ -m "not notebook and not pyomo" -n auto (CI's pr-tests invocation) -> 300 passed, 2 skipped in 371s (exit 0); the 2064 warnings are pre-existing opt_Pch.py optimizer warnings, unrelated to this PR
• Independent parity verification against Julia paramfits.jl @ f452ad4: KBBTransform matches KBB_transform_basic (guess * exp(theta) for Kvwf/Bf/Bvw), and BoundedKBBTransform matches KBB_transform_bounded (guess * scalefac * logistic(theta + logit(1/scalefac)), defaults 1e2/1e4/1e4). I checked the bounded values numerically against an independent implementation: theta=0 -> guess, theta=+1000 -> capped at guess*scalefac, theta=-1000 -> 0, and theta=2 matches to full precision. The shared-helper generalization is sound: _solve_primary_drying dispatches on RFParams vs PikalParams, _replace_params is now dataclass-generic, err_expT/obj_expT reuse applies (RF state index 1 = T_f, index 2 = T_vw), and the added terminated_by_drying validity gate only affects RF solutions, so conventional fitting is untouched.

Setup note: diff against merge-base (ad75ed6) matches the PR's reported totals exactly (4 files, +439/-33).

5. Intent vs issue #46

Closes #46 is appropriate (not merely Refs). The PR delivers the issue's scope: KBBTransform and BoundedKBBTransform (the bounded logistic variant), reuse of gen_sol_pd/obj_pd/err_expT/PrimaryDryFit via the generalized helpers, fit_rf_primary_drying(..., method="least_squares"|"minimize"), vial-wall time-series and endpoint fitting through PrimaryDryFit (both T_f and T_vw residual blocks are exercised), and preserved NaN behavior for invalid parameter sets (tested with badprms). Acceptance criteria are covered: least-squares recovery end to end, both residual blocks participate, and conventional fitting is unaffected; the minimize-recovery criterion is covered as noted in the nonblocking item above. The generic helpers stay parameterized rather than RF-specific, per the compatibility note.

Merge-readiness

No blocking issues; intent matches the issue; the KBB transforms and the Pikal/RF-shared fitting path are a faithful match to the Julia source, verified statically and numerically. Mergeable on the merits. The nonblocking test-coverage item and the multi-experiment question are refinements that do not gate merge. Formal approval must be recorded by another maintainer given the self-review restriction.

[bernalde]

This verifies the minimize wiring by monkeypatching minimize to return the exact solution, so it does not exercise an actual minimize-based recovery of Kvwf/Bf/Bvw. Issue #46's first acceptance criterion points at the Julia minimize path (test/test_RF_opt.jl#L61). The least-squares test recovers end to end and obj_rf is shown to be 0 at the exact point, so the substance is covered; consider adding one cheap end-to-end minimize recovery (low maxiter) so the optimizer path itself is regression-guarded. (Nonblocking.)

[bernalde]

Addressed in 4a493d0. The wiring-only monkeypatch test was replaced with tests/test_rf.py::test_fit_rf_primary_drying_minimize_recovers_kbb, which exercises the real SciPy minimize path and verifies recovered RF fit parameters within the requested tolerances.

[bernalde]

fit_rf_primary_drying takes a single RFParams/PrimaryDryFit, while the generic fit_primary_drying it calls (and gen_nsol_pd/objn_pd) support multi-experiment sequences and the matrix now calls gen_nsol_pd a "Multi-experiment Pikal/RF fitting helper." Confirm single-experiment-only is the intended RF convenience surface (multi-experiment RF via fit_primary_drying directly); if so, a one-line docstring note would set expectations. (Question.)

[bernalde]

Addressed in 4a493d0. The fit_rf_primary_drying docstring now states that it fits one RF experiment and directs multi-experiment RF fitting to fit_primary_drying.

[bernalde]

Addressed the current unresolved review comments.

• Commits pushed: 4a493d08d5e75c4051cc72bf0e4bbee5f8b95231 (Address RF fitting review coverage)
• Main changes: replaced the monkeypatched RF minimize test with a real fit_rf_primary_drying(..., method="minimize", optimizer_method="Nelder-Mead") run that checks optimizer success, real objective evaluations, objective improvement, and recovered Kvwf/Bf/Bvw ratios within the issue tolerances.
• Main changes: updated fit_rf_primary_drying docstring to state that it is the single-experiment RF convenience wrapper and that multi-experiment RF fitting should use fit_primary_drying directly.
• Tests: python -m ruff check lyopronto/fitting.py tests/test_rf.py passed.
• Tests: external-root focused RF minimize test passed, 1 passed in 17.67s.
• Tests: external-root tests/test_rf.py passed, 10 passed in 65.07s.
• Tests: external-root tests/test_primary_drying_fit.py tests/test_fitting.py passed, 11 passed in 56.59s.
• Tests: external-root CI-shaped suite pytest ... tests -n auto -v -m "not notebook and not pyomo" passed, 300 passed, 2 skipped in 383.89s.
• Tests: direct pytest from the hyphenated /tmp/LyoPRONTO-issue-46 worktree still fails during collection on the top-level __init__.py import artifact; the same tests were rerun from the external pytest root above.
• Type check: python -m mypy lyopronto cannot run directly from the hyphenated worktree for the same package-name issue. From a valid temporary root, Mypy failed on missing SciPy stubs plus existing project typing errors, including pre-existing fitting.py annotations; I did not weaken or skip that check.
• PR checks after push: SciPy Tests, doctests, and Deploy docs are pending; optional Pyomo is skipped.
• Comments intentionally not addressed: none.
• Remaining risks: the real RF minimize test starts near the known solution to keep runtime bounded; it verifies the public minimize path with SciPy but is not a broad convergence test from arbitrary distant guesses.