Skip to content

Code review: bug fixes, MPI tests, API hygiene - #58

Merged
Giovanni3A merged 3 commits into
mainfrom
jg/code-review
Sep 13, 2026
Merged

Giovanni3A merged 3 commits into
mainfrom
jg/code-review

Conversation

@joaquimg

@joaquimg joaquimg commented Aug 9, 2026

Copy link
Copy Markdown
Member

Coverage 67.4% → 94.0%. Both MPI optimizers were at 0%, which is where the worst bug was.

Bug fixes

  • Both MPI modes were unusable. They call _compute_single_step_cost directly, but only compute_cost called buildKeyError: :assess_policy_fix. build now runs in train!.
  • print(model) always threw — read model.forecast.network, field is networks.
  • JuMP.num_constraints(::Model) always threw — missing required count_variable_in_set_constraints kwarg.
  • .plan/.assess flattened N-D arrays and dropped DenseAxisArray axes (v[i=2:4].plan[2] returned the wrong element). Now map.
  • train! mutated the caller's Options (filter!/delete!), so reuse changed behaviour.
  • Options(GradientMode()) threw TypeError instead of ArgumentError.
  • _solve_bilevel unsupported-layer branch threw UndefVarError (ilayer vs i_layer).
  • MPI gradients could alias a shared buffer — now copied.
  • dCdz/dCdy were Float32 while duals/sensitivities are Float64 → silent precision loss.
  • Base.print / show_*_summary ignored io, wrote to stdout.
  • forecast was exported but never defined.
  • README example threw MethodError (train! took no Matrix for Y).

Breaking

  • SolutionSolution{C<:Real,P<:Real} (was Vector{Real}, boxed every param).
  • set_optimizer: dropped the unused third arg evaluate_duals.
  • 23 internals _-prefixed (docstrings kept, removed from API docs).
  • forecast removed from export.

Added

  • train! accepts Y::Matrix; both entry points error clearly if no forecast model is set.
  • PredictiveModel.networksAbstractVector: mixed model types and non-Dense layers (Flux.Scale, Chains with a bare activation) now work. flux_utils widened to match.

Tests

  • test/test_mpi.jl launches test/mpi/mpi_modes.jl under MPI.mpiexec(). Each MPI mode is asserted to match its serial counterpart from an identical start (8 checks, stochastic case seeded). Coverage flags propagate to the ranks. ADL_SKIP_MPI_TESTS=1 skips.
  • test/test_api.jl: 86 assertions — JuMP interface, Options, bound warnings, array shape/axes, flux_utils, aggregate=false, bilevel-after-build, matrix/dict train!, mixed & non-Dense layers.
  • Removed empty test/utils.jl and unused Distributions dep.
File %
gradient_mpi.jl 84.0
bilevel.jl 89.2
nelder_mead_mpi.jl 92.3
simulation.jl 92.9
ApplicationDrivenLearning.jl 95.2
flux_utils.jl 96.0
predictive_model.jl / nelder_mead.jl / gradient.jl / options.jl / solution.jl / jump.jl 100
TOTAL 93.99

Remaining gaps are solver-failure catch blocks and MPI early-exits that have covered serial twins.

Docs

  • Wrote mpi_modes.md (was empty, unreferenced). Documents that mpi_finalize=true (the default) makes JQM.is_controller_process() unusable after train!.
  • Fixed docstring signatures that didn't match the code; replaced the non-runnable PredictiveModel example (duplicate Dict keys, wrong types, wrong arity).
  • Documented that batch_size samples with replacement.
  • README: broken example, Pkg.add syntax, stale "not registered" claim.
  • Typos; VariableIndexedVector in newsvendor.md no longer exists; escaped R\$10/MWh (parsed as interpolation).
  • Docs build verified end-to-end, no broken refs.

CI

  • format_check.yml triggered on master; default branch is main (so it only ran on PRs).
  • Added Julia 1.10 to the matrix, matching the declared compat bound.

Dead code

trace array (written, never read), the T×epochs batches matrix in the deterministic path, unconditional "Final solution:" print.

Not changed

scheduling.md assess objective uses 10*y.assess (demand) where plan uses 10*z.plan (dispatch). Documented output is self-consistent with it, so intent is unclear — maintainer call.

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.74797% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.00%. Comparing base (c8116c3) to head (6228682).

Files with missing lines Patch % Lines
src/optimizers/nelder_mead_mpi.jl 81.81% 2 Missing ⚠️
src/ApplicationDrivenLearning.jl 96.77% 1 Missing ⚠️
src/optimizers/bilevel.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #58       +/-   ##
===========================================
+ Coverage   67.40%   94.00%   +26.60%     
===========================================
  Files          12       12               
  Lines         589      617       +28     
===========================================
+ Hits          397      580      +183     
+ Misses        192       37      -155     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joaquimg joaquimg mentioned this pull request Aug 16, 2026

@Giovanni3A Giovanni3A left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Giovanni3A
Giovanni3A merged commit 172340f into main Sep 13, 2026
8 checks passed
@Giovanni3A
Giovanni3A deleted the jg/code-review branch September 13, 2026 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants