Add solver barrier for VEP and fix is_viscoplastic - #95
Conversation
- Add requires_stress_history property to Constitutive_Model base (False) and override in ViscoElasticPlasticFlowModel (True). - Stokes solver setter raises TypeError when assigned a constitutive model that requires stress history but DFDt is not available. Prevents silent failure where VEP on plain Stokes drops all history terms. - Fix is_viscoplastic: was comparing UWexpression == sympy.oo (always False due to type mismatch); now uses .sym is sympy.oo. Underworld development team with AI support from Claude Code
There was a problem hiding this comment.
Pull request overview
Adds two safety fixes around the visco-elastic-plastic (VEP) constitutive model to prevent silent loss of stress-history terms and to correct a yield-stress infinity check.
Changes:
- Add a solver-side guard that raises a
TypeErrorwhen a stress-history constitutive model is assigned to a solver withoutDFDtsupport (guiding users touw.systems.VE_Stokes). - Introduce a
requires_stress_historycapability flag on constitutive models (defaultFalse,Truefor VEP). - Fix
ViscoElasticPlasticFlowModel.is_viscoplasticto correctly detect the “no yielding” case whenyield_stressis infinite.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/underworld3/cython/petsc_generic_snes_solvers.pyx |
Adds an early runtime barrier when assigning incompatible constitutive models (stress-history requirement without DFDt). |
src/underworld3/constitutive_models.py |
Adds the requires_stress_history property and corrects is_viscoplastic infinity detection for VEP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| f"(DFDt). Use uw.systems.VE_Stokes instead of uw.systems.Stokes, or provide " | ||
| f"a DFDt object when constructing the solver." |
There was a problem hiding this comment.
The error message suggests that providing a DFDt object to uw.systems.Stokes is a supported alternative, but plain Stokes doesn’t perform the stress-history DFDt.update_pre_solve()/update_post_solve() sequencing that VE_Stokes does. This can mislead users into thinking history will be handled correctly when it won’t. Consider removing that suggestion, or explicitly stating that manual DFDt management is required if not using VE_Stokes (and/or tightening the check to require a solver that actually manages stress history).
| f"(DFDt). Use uw.systems.VE_Stokes instead of uw.systems.Stokes, or provide " | |
| f"a DFDt object when constructing the solver." | |
| f"(DFDt). Use uw.systems.VE_Stokes instead of uw.systems.Stokes. If you " | |
| f"provide a DFDt object when constructing a non-VE solver, you must manually " | |
| f"call DFDt.update_pre_solve() and DFDt.update_post_solve() each timestep." |
| if self._constitutive_model.requires_stress_history and self.Unknowns.DFDt is None: | ||
| raise TypeError( | ||
| f"{type(self._constitutive_model).__name__} requires stress history tracking " | ||
| f"(DFDt). Use uw.systems.VE_Stokes instead of uw.systems.Stokes, or provide " | ||
| f"a DFDt object when constructing the solver." | ||
| ) |
There was a problem hiding this comment.
This new runtime guard changes behavior for existing code/tests that currently set ViscoElasticPlasticFlowModel on uw.systems.Stokes (e.g. tests/test_1050_VEstokesCart.py uses plain Stokes). Please add/adjust pytest coverage to (a) assert the new TypeError on plain Stokes, and (b) update any existing tests/examples to use VE_Stokes (or to expect the error), so CI doesn’t regress silently.
| """Whether this model needs DFDt stress history tracking. | ||
|
|
||
| Models that return True require a solver with stress history | ||
| management (e.g. VE_Stokes). Assigning such a model to a plain | ||
| Stokes solver will raise an error. |
There was a problem hiding this comment.
The docstring says assigning a stress-history model to a plain Stokes solver “will raise an error”, but the enforcement depends on solver-side checks and can be bypassed (e.g. if Unknowns.DFDt is non-None). Consider rewording this to reflect the actual contract (e.g. “requires a solver that manages stress history such as VE_Stokes; plain Stokes is unsupported”) so it stays accurate even if the solver-side guard changes.
| """Whether this model needs DFDt stress history tracking. | |
| Models that return True require a solver with stress history | |
| management (e.g. VE_Stokes). Assigning such a model to a plain | |
| Stokes solver will raise an error. | |
| """Whether this model needs DFDt stress-history tracking. | |
| Models that return True require a solver that manages stress | |
| history (e.g. VE_Stokes). Using such a model with a plain Stokes | |
| solver is unsupported and may fail depending on the solver | |
| configuration. |
The test used plain Stokes with ViscoElasticPlasticFlowModel, which silently dropped stress history terms. Now uses VE_Stokes with timestep=0.1, matching the dt_elastic=1/10 already set in the test. Required by PR #95 (VEP solver barrier) and PR #97 (solver unification) which correctly reject VEP models on plain Stokes. Underworld development team with AI support from Claude Code
The test used plain Stokes with ViscoElasticPlasticFlowModel, which silently dropped stress history terms. Now uses VE_Stokes with timestep=0.1, matching the dt_elastic=1/10 already set in the test. Required by PR #95 (VEP solver barrier) and PR #97 (solver unification) which correctly reject VEP models on plain Stokes. Underworld development team with AI support from Claude Code
The test used plain Stokes with ViscoElasticPlasticFlowModel, which silently dropped stress history terms. Now uses VE_Stokes with timestep=0.1, matching the dt_elastic=1/10 already set in the test. Required by PR #95 (VEP solver barrier) and PR #97 (solver unification) which correctly reject VEP models on plain Stokes. Underworld development team with AI support from Claude Code
Conflicts resolved in favour of solver-unification (PR #97): - petsc_generic_snes_solvers.pyx: keep auto-create DFDt (lazy adaptation) over PR #95's TypeError barrier (superseded by unification) - constitutive_models.py: keep new properties (yield_mode, yield_softness, bdf_blend, plastic_fraction) from solver-unification Underworld development team with AI support from Claude Code
Summary
Two small safety fixes for the VEP constitutive model, additive to PR #89.
1. Solver barrier for stress history models
Assigning
ViscoElasticPlasticFlowModelto a plainuw.systems.Stokessolver silently drops all stress history terms — the solver runs without error but produces purely viscous results. This was a known failure mode (e.g.test_1050_VEstokesCart.pyandEx_Sheared_Layer_Test.pyboth used plain Stokes with VEP).Now raises
TypeErrorwith a clear message directing users toVE_Stokes.2. Fix
is_viscoplasticcomparisonis_viscoplasticcomparedself.Parameters.yield_stress == sympy.oobutyield_stressis aUWexpression, so the comparison always returnedFalse. Now uses.sym is sympy.oo.Test plan
is_viscoplasticreturns False when yield_stress is infiniteUnderworld development team with AI support from Claude Code