fix: neural optimizer reports actual convergence instead of hardcoded 0L (#107) - #115
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Fix is correct. Reviewed all changed files (Python optimizer, Python tests, R optimizer, R tests).
Python (optimize.py):
"converged": stall >= patience is the right expression. The training loop exits when stall >= patience (plateau detected → early stop) or when the epoch budget is exhausted. After the loop, stall >= patience is True only in the first case. This mirrors the parametric Adam branch that already had this logic (per the PR description referencing #92).
R (optimizer.R):
convergence <- if (isTRUE(result$converged)) 0L else 1L follows R's optim() convention (0 = success, 1 = failure/non-convergence). The isTRUE() guard handles NULL safely if Python ever returns a result without the converged key.
Tests:
- Python:
test_not_converged_when_epochs_exhausted_while_improvingusesn_epochs=5, patience=1000— the budget runs out before any plateau, sostall < patienceat loop exit →converged=False. ✓ - Python: existing
test_mlp_early_stoppingnow also assertsconverged=True(zero LR → no progress → stall reaches patience). ✓ - Python:
test_mlp_returns_expected_keysupdated to expect"converged"in the result dict. ✓ - R: Two mocked tests cover the
True→0LandFalse→1Lmappings cleanly.
CI: all R-CMD-check and Python CI checks passing. ✅
LGTM.
Closes #107
Problem
run_neural_optimization()'s Adam training loop already tracksstall/patienceand early-stops on plateau, but the dict it returned never included aconvergedflag..optimize_neural()in R/optimizer.R then hardcodedconvergence = 0L, so anymlp/conv/spline_gam/irlfit that ran out of its epoch budget while still improving was reported as converged — indistinguishable from an actual plateau. This is the same failure mode #92 fixed on the parametric (Adam) path.Fix
run_neural_optimization()now returns"converged": stall >= patience(mirrors the parametric Adam branch)..optimize_neural()now mapsconvergence <- if (isTRUE(result$converged)) 0L else 1Linstead of the literal0L.Tests
test_not_converged_when_epochs_exhausted_while_improvingtotest_neural_optimize.py, and added"converged"to the expected-keys set intest_mlp_returns_expected_keys; extendedtest_mlp_early_stoppingto assertconverged is True.testthatcases intest-optimizer.Rmockingds_jax_neural_optimize()to returnconverged = TRUE/converged = FALSEand asserting.optimize_neural()'sconvergencefield maps to0L/1Lrespectively.Ran locally:
inst/python/diffiscape_jax/tests/test_neural_optimize.py— 11 passed.tests/testthat/test-optimizer.RwithNOT_CRAN=true— 70 passed, 0 failed.