estimate_dt: opt-in median/percentile cell reduction (sliver-robust dt)#220
Open
lmoresi wants to merge 1 commit into
Open
estimate_dt: opt-in median/percentile cell reduction (sliver-robust dt)#220lmoresi wants to merge 1 commit into
lmoresi wants to merge 1 commit into
Conversation
estimate_dt reduced the per-element advective/diffusive dt with np.min, so a single anisotropic sliver cell (velocity across its short dimension) collapses the global dt. On adaptive-mesh runs this froze the timestep (~250x collapse observed at an adapt), stalling the run while the metric kept refining a frozen field. SLCN is unconditionally stable, so the typical cell should set dt, not the worst sliver. Add an opt-in `percentile` kwarg (default 0.0 = strict global min, fully backward-compatible). percentile>0 reduces the per-element dt via the Nth global percentile (50 = median; allgather finite values + np.percentile) instead of np.min. Combined with the existing direction_aware extent (cell size ALONG the flow), this gives an orientation-aware + sliver-robust dt. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the advection–diffusion SLCN solver’s estimate_dt() to optionally reduce per-element timestep estimates via a global percentile (e.g., median) instead of a strict global minimum, preventing a small number of anisotropic “sliver” cells from collapsing the simulation timestep on adapted meshes.
Changes:
- Add
percentile: float = 0.0(opt-in) toestimate_dt()to compute the Nth global percentile of finite per-elementdtvalues (default remains strict global min for backward compatibility). - Refactor the global dt reduction logic into a local helper (
_reduce_dt) and apply it to both advective and diffusive per-element timestep arrays.
Comments suppressed due to low confidence (1)
src/underworld3/systems/solvers.py:3176
- The new
percentilekwarg isn’t documented in the docstring’s Parameters section, and the narrative still implies a strict minimum reduction. This makes it hard for users to discover the new behavior and understand the valid range/parallel cost.
def estimate_dt(self, direction_aware: bool = False, percentile: float = 0.0):
r"""
Estimate an appropriate timestep for the advection-diffusion solver.
This is an implicit solver so the returned :math:`\delta t` is the
minimum of:
- :math:`\delta t_{\textrm{diff}}`: typical time for diffusion across an element
- :math:`\delta t_{\textrm{adv}}`: typical element-crossing time for a fluid parcel
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3278
to
+3288
| # Reduce per-element dt to one global value. Default (percentile=0) = | ||
| # strict global MINIMUM — one cell sets the limit. percentile>0 takes the | ||
| # Nth global percentile (50 = median) of the per-element dt instead, so a | ||
| # few anisotropic SLIVER cells (velocity ACROSS a thin cell) don't collapse | ||
| # dt. SLCN is unconditionally stable, and ``direction_aware`` already | ||
| # credits cells stretched ALONG the flow — together they give the | ||
| # orientation-aware + sliver-robust timestep. | ||
| def _reduce_dt(per_elem): | ||
| fin = per_elem[np.isfinite(per_elem)] if len(per_elem) else per_elem | ||
| if percentile and percentile > 0: | ||
| gathered = comm.allgather(np.ascontiguousarray(fin, dtype=float)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AdvDiffusionSLCN.estimate_dtreduces the per-element advective/diffusive timestep withnp.min. A single anisotropic sliver cell (velocity across its short dimension) then sets the globaldt. On adaptive-mesh runs the mover routinely produces a few thin cells, which collapseddtby ~250× at an adapt — freezing the simulation while the metric kept refining a frozen field. SLCN is unconditionally stable, so the typical cell should setdt, not the worst sliver.Fix
Add an opt-in
percentilekwarg toestimate_dt:percentile=0.0(default) → strict globalmin, bit-for-bit backward compatible.percentile>0→ reduce the per-elementdtvia the Nth global percentile (50 = median;allgatherof finite values +np.percentile) instead ofnp.min.Combined with the existing
direction_aware=True(per-cell extent along the velocity), this gives an orientation-aware and sliver-robust timestep.Validation
Used throughout a 50-step adaptive convection run (Ra=1e7, anisotropic MMPDE mover, adapt-every-step) at
percentile=50:dtno longer collapses (held ~3e-4 vs ~1.9e-6 with min), the run advances normally serial and at np=5. Serial default path (percentile=0) unchanged.Underworld development team with AI support from Claude Code