Increase size of porous convection problem - #344
lukem12345 wants to merge 1 commit into
Conversation
|
I just tagged this as a "good first issue", but this may require access to some compute on hipergator. |
|
PR 196 of CombinatorialSpaces is now merged, so we can execute some multigrid simulations via the Galerkin technique now. |
There was a problem hiding this comment.
Pull request overview
Adjusts the porous convection example script to support benchmarking runs (primarily via solver selection, logging, and saving solution outputs).
Changes:
- Moves solver selection (
solvername) to the top of the script and adds a number of@infoprogress logs. - Saves a single solver run’s temperature history and metadata to a solver-specific
.matfile. - Comments out a large downstream block of multi-solver comparison/plotting code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,8 @@ | |||
| # GMRES, Direct_LU, MG, GMRES_MG, AMG | |||
There was a problem hiding this comment.
The header comment lists "AMG" as a supported solver option, but there is no AMG branch below and choosing solvername="AMG" will leave Δ⁻¹ undefined (leading to runtime errors when generating the simulation). Either implement the AMG option, remove it from the list, and/or add an explicit else that errors on unknown solver names.
| # GMRES, Direct_LU, MG, GMRES_MG, AMG | |
| # GMRES, Direct_LU, MG, GMRES_MG |
| @@ -218,6 +222,7 @@ elseif solvername == "GMRES_MG" | |||
| Δ⁻¹(x) = begin y, _ = Krylov.gmres(Δ0, x; M = md, ldiv = true); y .-= minimum(y) end | |||
| end | |||
There was a problem hiding this comment.
The solver selection chain has no final else branch. If solvername is set to an unsupported value, Δ⁻¹ will never be defined and the script will fail later in generate. Add an explicit else that throws a clear error (and consider validating solvername near its definition).
| function temperature_matrix(x) | ||
| reduce(hcat, map(x) do v | ||
| v.T | ||
| end) | ||
| end | ||
|
|
||
| matwrite("solns_$(solvername).mat", | ||
| Dict( | ||
| "soln" => temperature_matrix(soln), | ||
| "nf" => soln.stats.nf, | ||
| "time" => soln.t, | ||
| "time_s" => time_s, | ||
| "reltol" => reltol)) |
There was a problem hiding this comment.
temperature_matrix + matwrite materializes the full state history into a dense matrix (nv(sd) × nt). With the larger mesh this can easily become a multi-GB allocation and dominate benchmark runtime or crash due to OOM. Consider writing time slices incrementally, saving only a subset of timesteps, or using a format better suited for large arrays (e.g., HDF5/JLD2 with chunking) instead of eagerly concatenating all columns.
| #= | ||
| matwrite("all_solns_aug13_2025.mat", | ||
| Dict( | ||
| "directlu" => temperature_matrix(soln_directlu), | ||
| "decgmg" => temperature_matrix(soln_decgmg), | ||
| "gmres" => temperature_matrix(soln_gmres), | ||
| "nf_directlu" => (soln_directlu.stats.nf), | ||
| "nf_decgmg" => (soln_decgmg.stats.nf), | ||
| "nf_gmres" => (soln_gmres.stats.nf), | ||
| "time" => soln_directlu.t, | ||
| "time_decgmg" => time_decgmg, | ||
| "time_directlu" => time_directlu, | ||
| "time_gmres" => time_gmres, | ||
| "reltol" => 1e-9)) | ||
| "directlu" => temperature_matrix(solns["Direct_LU"]), | ||
| "decgmg" => temperature_matrix(solns["MG"]), |
There was a problem hiding this comment.
A very large portion of the script is now commented out with a #= ... =# block. Keeping substantial dead code in-place makes the example harder to maintain and can be confusing for benchmark runs (it’s unclear what is intended to execute). Consider moving this analysis/plotting code into a separate file (or guarding it behind a flag/CLI arg) instead of commenting it out.
| # GMRES, Direct_LU, MG, GMRES_MG, AMG | ||
| solvername = "MG" | ||
| #solvername = "Direct_LU" |
There was a problem hiding this comment.
PR description says this increases the porous convection problem size so direct solvers become infeasible, but in this diff the core sizing parameters (e.g., subs, domain extents, Ra, end time) don’t appear to change; most updates are logging/output and restructuring. Either update the PR description to match the actual change, or include the intended sizing change in this PR.
For benchmarking purposes, we should execute the porous convection simulation in a regime where direct solvers are infeasible.