Skip to content

Update adapt cost microbenchmark for current APIs - #476

Open
ss2098 wants to merge 3 commits into
underworldcode:developmentfrom
ss2098:ss2098/update-adapt-cost-microbench
Open

Update adapt cost microbenchmark for current APIs#476
ss2098 wants to merge 3 commits into
underworldcode:developmentfrom
ss2098:ss2098/update-adapt-cost-microbench

Conversation

@ss2098

@ss2098 ss2098 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This PR updates scripts/adapt_cost_microbench.py so the microbenchmark runs with the current Underworld3 APIs.

Changes include:

  • convert AdvDiffusionSLCN estimate_dt() output to a scalar before passing it to solve()
  • replace internal mesh._deform_mesh() calls with the public mesh.deform() API
  • replace the retired smooth_mesh_interior(..., method="anisotropic") path with uw.meshing.node_redistribution()
  • add fail-fast SNES convergence checks so the benchmark does not continue after a failed solve
  • simplify the Stokes boundary condition so the microbenchmark runs stably as a cost diagnostic

This does not change solver internals.

Validation:

  • pixi run python scripts/adapt_cost_microbench.py

Example local output:

  • plain (adv+stokes) step: 1.217 s
  • one pristine adaptation total: 3.586 s
  • adaptation / step ratio: 2.9x

@ss2098
ss2098 requested a review from lmoresi as a code owner July 29, 2026 08:11

@lmoresi lmoresi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adversarial review

The API modernizations are correct (we live-probed node_redistribution, metric_density_from_gradient(amp=, name=), Mesh.deform, .snes — all present), but two substitutions change what the benchmark measures:

  1. MAJOR — the upper boundary changed from penalty free-slip to no-slip (add_essential_bc((0,0))). That silently changes the convection physics while the docstring still claims "same setup as the saturation runner", so the numbers are no longer comparable to the recorded baselines this script exists to track. add_natural_bc still exists; the sanctioned modernization is add_rotated_freeslip_bc(0, "Upper").
  2. MAJOR — the timed Stokes solves flipped zero_init_guess=False → True: the benchmark now times cold-start solves, inflating t_step and deflating the adaptation/step cost ratio.
  3. MINOR — _deform_mesh → deform: deform() runs remesh_with_field_transfer itself before the script overwrites T, so the remap timings now double-count transfer work.

Requested changes: restore free-slip via add_rotated_freeslip_bc(0, "Upper"), restore zero_init_guess=False in the timed loops (or explicitly re-baseline and relabel), and note the deform double-transfer in the timing comment. With those, this is mergeable — the rest of the update checked out clean.

@ss2098
ss2098 force-pushed the ss2098/update-adapt-cost-microbench branch from d581610 to 12979fe Compare August 10, 2026 04:31
@ss2098

ss2098 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, Professor @lmoresi . I restored the intended upper free-slip boundary using add_rotated_freeslip_bc(0, "Upper") and restored warm-start Stokes timing with zero_init_guess=False.

I also added a note that Mesh.deform() includes remesh_with_field_transfer internally, so the remap timing includes that transfer cost before the script overwrites fields.

Validation:

  • python -m py_compile scripts/adapt_cost_microbench.py
  • pixi run python scripts/adapt_cost_microbench.py

@lmoresi

lmoresi commented Aug 12, 2026

Copy link
Copy Markdown
Member

Close — two of three verified, but it looks like the free-slip fix landed on #479's file rather than this one. At the current head (12979fe), the diff contains the four zero_init_guess=False changes (verified, thank you), while scripts/adapt_cost_microbench.py still has:

Push those same two edits here and this is mergeable — the script already runs clean against current development (we smoke-ran it, exit 0), and your #479 and #480 are merged.

Underworld development team with AI support from Claude Code

@ss2098

ss2098 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks Professor @lmoresi. I pushed the remaining two edits to this PR.

Changes:

  • restored the upper free-slip boundary with add_rotated_freeslip_bc(0, m.boundaries.Upper.name)
  • added the Mesh.deform() / remesh_with_field_transfer timing note near the remap timing section

I had been thinking about this comparison issue after fixing #479, but waited for your comment here before making the same change on this branch.

Validation:

  • python -m py_compile scripts/adapt_cost_microbench.py
  • pixi run python scripts/adapt_cost_microbench.py

Current local output:

  • plain adv+stokes step: 1.206 s
  • one pristine adaptation total: 4.122 s
  • adaptation / step ratio: 3.4x
  • amortised adapt-every-5 overhead: +0.7x

@lmoresi

lmoresi commented Aug 18, 2026

Copy link
Copy Markdown
Member

Adversarial review — documentation, skills and tooling (#476, #580, #598, #599)

Reviewed together because none of them changes library behaviour and the useful
question is the same for all four: is the claim they make true of the code as it
stands today.

#580refinement=R is a factor on the background spacing

Verified against the source. The correction says coarsening="auto" is the
budget-conserving R**(1/d), so the envelope is h in [h0/R, h0·R**(1/d)] and
the finest:coarsest ratio is R**(1+1/d). metrics.py:587 reads

coar_val = ref_val ** (1.0 / cdim)

which is exactly that, and the arithmetic in the text checks: R=5 at d=2 gives
2.236 and a ratio of 11.18; at d=3, 1.71 and 8.55. The previous wording — "the
finest:coarsest grading ratio" — was wrong by that factor, so anyone who tuned R
against an observed ratio was tuning against a number roughly twice what they
asked for.

One thing the correction does not say: whether any existing example or notebook
was written against the old reading and now wants its R adjusted. Worth a grep
before this lands, since the doc change alone will make previously-tuned scripts
look wrong rather than making them right.

#476 — adapt cost microbenchmark

Its red CI is stale, not a defect in the change. The failures are in
test_0851_fault_network_3d.py and test_0851_std_reduction_method.py, neither
of which this PR touches — it changes one script. The run is from 2026-08-12
against a trunk that was red at the time. We re-triggered it; if it comes back
green the PR is a one-file update with nothing to argue about.

scalar_dt is the substantive addition: it coerces estimate_dt() output to a
float, taking nanmin over an array, and raises on a non-finite or non-positive
result. That is a benchmark script defending itself against an API that returns
different shapes, which is reasonable here — but the same coercion is what a
caller of estimate_dt() in a model script would have to write, and it belongs
behind the API rather than in each consumer. Worth an issue rather than a change
to this PR.

#598, #599 — cetz figure skill and the rotated-basis figure

These two are a pair: #598 corrects the skill's anchor guidance and adds
gotchas, #599 is a figure built to that skill's rule that geometry is computed
in Python and Typst only draws. #599's own docstring records why — an earlier
version connected nodes by a distance threshold and silently dropped several
from the mesh — which is the kind of failure the rule exists to prevent, and it
is good that the script says so where the next author will read it.

The reviewable question for the pair is whether #598's corrected guidance
matches what #599 actually does, since one is the rule and the other is the
worked example. They were authored together, so agreement is likely and
unchecked; if the skill is meant to be normative it would be worth having the
example's JSON schema referenced from the skill rather than described twice.

Neither changes library code, so the risk is confined to what a future author is
told.

Underworld development team with AI support from Claude Code

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