Fix IPOPT result assembly and PCR prediction centering

[Mazhar331]

Summary

Closes #78.
Refs #67.

• Fix ParameterEstimation.optimize_fn(method='IPOPT') result assembly by using the base get_objective(..., set_self=False) keyword when collecting final weighted residuals.
• Fix PCR_calibration.predict() so non-SNV prediction inputs are centered and standardized with the training statistics stored during calibration.
• Add focused regression coverage for both documented failure modes.

Acceptance Criteria

• IPOPT result assembly no longer calls ParameterEstimation.get_objective() with the subclass-only update_self keyword.
• PCR prediction for a single new spectrum remains finite and matches an independent training-statistics projection calculation.
• Changed source comments/docstrings state the relevant contracts: weighted residuals are dimensionless after sigma_inv, PCR prediction inputs use the calibration data units, standardized predictors/scores are dimensionless, and predicted responses keep the fitted response units.

Review Notes

• The IPOPT regression test still uses monkeypatch for the optional external solver boundary only: have_cyipopt and minimize_ipopt.
• A fully unpatched core test cannot reach the fixed result-assembly branch in this environment because optimize_fn(method='IPOPT') raises the optional cyipopt ImportError before calling the final get_objective(..., set_self=False) path.
• Commit 8917417 removed the internal estimator.get_gradient monkeypatch. The test now supplies a real jac_fun, so PharmaPy's actual ParameterEstimation.get_gradient(), final weighted-residual assembly, y_model assembly, and covariance calculation run after the fake solver returns.
• Commit 34927aa adds module, test, and helper-callback docstrings plus unit comments for each dimensional fixture and derived intermediate in tests/test_paramestim_calibration_fit_predict.py.

Validation

• Red on the PR branch point (8c7d14d, the base at implementation time) with the final regression file copied into a temporary worktree:
  • python -m pytest tests/test_paramestim_calibration_fit_predict.py -q
  • Fails with the old IPOPT TypeError: ParameterEstimation.get_objective() got an unexpected keyword argument 'update_self'.
  • Fails with PCR single-spectrum nan prediction from row-local zero standard deviation.
• Review-response validation:
  • On 8917417, fully unpatched IPOPT probe exits with the optional cyipopt ImportError, confirming that a no-monkeypatch core test would not reach the regression branch.
  • On 8917417, temporarily reintroduced the old update_self=False call; python -m pytest tests/test_paramestim_calibration_fit_predict.py::test_parameter_estimation_ipopt_result_assembly_uses_base_keyword -q failed with the original TypeError, confirming the refined test still guards the bug.
  • On 34927aa, completed the units/docs/comments audit over the test diff: test/module/helper documentation is present; [s], [mol/L], [mol/L/s], [AU], [g/L], covariance units, and dimensionless [-] predictor/score/component quantities are annotated in the test file.
• Green on head 34927aa:
  • python -m pytest tests/test_paramestim_calibration_fit_predict.py -q -> 2 passed
  • python -m pytest --collect-only -q -> 69 tests collected
  • python -m pytest tests/ -m "not assimulo" -q -> 63 passed, 5 skipped, 6 deselected
  • python -m py_compile tests/test_paramestim_calibration_fit_predict.py
  • git diff --check

Branch Hygiene

• Base branch: master
• Source branch point: 8c7d14d (the base at implementation time; current origin/master has advanced since this PR branch was opened).
• Head branch: fix/issue-78-fit-predict-results
• Stacked status: not stacked; no prerequisite PRs.
• Open-PR overlap check: no open PRs currently touch PharmaPy/ParamEstim.py, PharmaPy/Calibration.py, or tests/test_paramestim_calibration_fit_predict.py.

[Mazhar331]

Maintainer review. This account is the PR author, so GitHub only allows a COMMENT verdict here; the findings below reflect a full review. There are no blocking findings — both fixes are correct, match the fixes prescribed in #78, and are guarded by tests that I verified fail on base.

What I verified

• IPOPT fix (ParamEstim.py:674) — the base get_objective(self, params, out_array=False, set_self=True) has no update_self parameter (only the MultipleCurveResolution subclass override does), so the pre-fix call raised TypeError right after a successful IPOPT solve. Switching to set_self=False both fixes the crash and correctly avoids overwriting the solved-state resid_runs that the final y_model assembly at ParamEstim.py:672 depends on.
• PCR fix (Calibration.py:245-246) — the non-SNV branch now centers/standardizes new inputs with the stored training data_mean/data_std instead of recomputing them from the prediction rows, so a single new spectrum no longer hits std=0 → divide-by-zero → nan, and multi-row batches project onto the fitted PC basis consistently.
• Tests genuinely guard the fixes. I reverted each production fix in a scratch copy (keeping the test) and re-ran: the IPOPT test failed with the exact TypeError: ... unexpected keyword argument 'update_self', and the PCR test failed with the divide by zero RuntimeWarning → nan. With the fixes restored, both pass. The PCR expected value is computed independently of the production expression (manual centering + score projection), not by duplicating predict().
• Docstrings/units are accurate — the added Numpy-format docstrings and the units annotations in the tests ([mol/L], [AU], dimensionless [-], [s], [g/L]) match the actual quantities at each boundary.

Findings

Nonblocking: SNV single-spectrum predict retains the same nan failure mode

The SNV branch (Calibration.py:243) still calls self.__center_data(inputs) with no stats, so for a single new spectrum it recomputes std=0 over that one row and returns nan — the identical failure this PR fixes for the non-SNV branch. I reproduced it directly: an snv=True model predicting one new spectrum yields [[nan]].

This is outside #78's stated scope (the issue prescribes fixing only the non-SNV branch, which this PR does correctly), so it does not block. But it is worth a follow-up: the correct SNV handling likely differs from the non-SNV fix — canonical SNV normalizes each spectrum by its own across-wavelength mean/std (a per-row operation), whereas the current __center_data uses axis=0 (across-sample) statistics, which is really column standardization. Recommend a follow-up issue to define and fix the SNV single-row contract rather than reusing the training-stats approach blindly.

Tests run

• pytest tests/test_paramestim_calibration_fit_predict.py -q → 2 passed (pharmapy conda env).
• Red-on-base confirmation: both tests fail with the documented symptoms when each fix is reverted.
• pytest tests/ -m "not assimulo" -q → 63 passed, 15 deselected — no regressions.

Merge readiness

• Changed-file set matches gh pr diff --name-only; reviewed at head 1ccf83b.
• master is unprotected (no branch-protection approval or status-check gate), so no maintainer-approval gate is enforced by GitHub. This account cannot post a formal APPROVE because it authored the PR; if a formal approval is desired, another maintainer account would need to provide it, but it is not required to merge.
• Closes #78 is accurate — both defects in #78 are fully resolved.

No blocking findings. The single Nonblocking item can be handled as a follow-up.

[Mazhar331]

Addressed the review batch at head 1ccf83b80f026cef5f890bdbe23a951b7b013966.

• Commits pushed: none; the review item is explicitly out of High: [ParamEstim/Calibration] IPOPT result assembly crashes and PCR predict recenters on new data #78's PR scope, and the branch was already up to date.
• Main action: opened follow-up issue SNV PCR prediction returns NaN for a single new spectrum #138 for the SNV single-spectrum PCR_calibration.predict() nan behavior, with the local reproduction and acceptance criteria.
• Verification: reproduced the SNV [[nan]] behavior on PR Fix IPOPT result assembly and PCR prediction centering #137 head, confirmed no duplicate SNV/PCR issue beyond High: [ParamEstim/Calibration] IPOPT result assembly crashes and PCR predict recenters on new data #78 showed up in issue search, and re-read SNV PCR prediction returns NaN for a single new spectrum #138 after creation.
• Checks: no-op push returned Everything up-to-date; gh pr checks 137 still reports Core tests and Assimulo integration tests passing.
• Intentionally not changed in this PR: the SNV implementation remains deferred to SNV PCR prediction returns NaN for a single new spectrum #138 because the review marked it Nonblocking/out of High: [ParamEstim/Calibration] IPOPT result assembly crashes and PCR predict recenters on new data #78 scope and the likely fix requires defining the SNV per-spectrum normalization contract.
• Remaining gate: the review decision remains comment-only/blank until a reviewer updates it; there are no blocking findings in the existing review.

[Mazhar331]

Addressed the monkeypatch review question at head 89174171c14e251e8cdab16d6bc245f206499a88.

• Commits pushed: 8917417 Clarify IPOPT regression test solver boundary.
• Resolution: narrowed the IPOPT regression test so it patches only the optional cyipopt/minimize_ipopt solver boundary. It no longer monkeypatches estimator.get_gradient; instead it supplies a real jac_fun and asserts the returned Jacobian, so PharmaPy's real gradient, residual, y_model, and covariance assembly paths run after the fake solver returns.
• Justification: a fully unpatched core test raises the optional cyipopt ImportError before reaching the fixed result-assembly branch. Running real IPOPT would make the core regression test depend on an optional native solver stack rather than the bug's post-solve assembly contract.
• PR metadata: updated the body with the monkeypatch rationale and review-response validation.
• Verification: python -m pytest tests/test_paramestim_calibration_fit_predict.py -q -> 2 passed; python -m pytest --collect-only -q -> 69 tests collected; python -m pytest tests/ -m "not assimulo" -q -> 63 passed, 5 skipped, 6 deselected; git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --check; mutation check by temporarily restoring update_self=False failed with the original TypeError.
• CI on the pushed head is green: Core tests and Assimulo integration tests both passed.
• Remaining gate: reviewDecision is still CHANGES_REQUESTED until the reviewer updates or dismisses it.

[Mazhar331]

Addressed the test documentation/units convention at head 34927aac727b406effca74452ebb4f2b086d3b4f.

• Commit pushed: 34927aa Document regression test units.
• Resolution: added a module docstring, test docstrings, helper-callback docstrings, and unit comments/names for the test fixtures and derived values in tests/test_paramestim_calibration_fit_predict.py.
• Units covered in the test file: [s], [mol/L], [mol/L/s], [AU], [g/L], covariance rate-variance units [(mol/L/s)^2], and dimensionless [-] normalized predictors, PC loadings/scores, and component count.
• Verification: python -m pytest tests/test_paramestim_calibration_fit_predict.py -q -> 2 passed; python -m pytest --collect-only -q -> 69 tests collected; python -m pytest tests/ -m "not assimulo" -q -> 63 passed, 5 skipped, 6 deselected; python -m py_compile tests/test_paramestim_calibration_fit_predict.py; git diff --check.
• PR metadata: updated the body with the docs/units audit and current-head validation.
• CI on the pushed head is green: Core tests and Assimulo integration tests both passed.
• Remaining gate: reviewDecision is still CHANGES_REQUESTED until the reviewer updates or dismisses it.