-
Notifications
You must be signed in to change notification settings - Fork 96
feat: oscillation scaling #3745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
localIndex const historySize = m_solutionHistory.size(); | ||
|
||
RAJA::forall< parallelDevicePolicy<> >( RAJA::TypedRangeSegment< localIndex >( 0, numDofs ), | ||
[&] GEOS_HOST ( localIndex const dof ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[&] GEOS_HOST ( localIndex const dof ) | |
[=] GEOS_HOST_DEVICE ( localIndex const dof ) |
GEOS_ERROR_IF_LT_MSG( m_oscillationCheckDepth, 2, | ||
getWrapperDataContext( viewKeysStruct::oscillationCheckDepthString() ) << ": can not be less than 2" ); | ||
GEOS_ERROR_IF_LE_MSG( m_oscillationTolerance, 0.0, | ||
getWrapperDataContext( viewKeysStruct::oscillationToleranceString() ) << ": should be positive" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also be less than 1?
} | ||
} ); | ||
|
||
real64 const f = static_cast< real64 >( MpiWrapper::sum( oscillationCount.get() ) ) / numDofs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also mpi sum numDofs
here.
GEOS_FMT( " {}: oscillation detected, scaling factor set to {}", getName(), scalingFactor ) ); | ||
} | ||
|
||
m_solutionHistory.appendArray( localSolution.begin(), localSolution.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to populate of m_nonlinearSolverParameters.m_oscillationScaling
is false.
@dkachuma all your comments should be fixed now, thanks |
@dkachuma example how it helps a bit for dome case:
![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A unit test or integrated test to exercise the detection logic would be good. Ideally this could be exercised in isolation from a full simulation run, but this isn't always possible..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i enabled it for one example from integrated tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces oscillation detection and scaling functionality for Newton iteration levels in physics solvers. The feature helps stabilize convergence by automatically scaling solution changes when oscillatory behavior is detected.
Key changes include:
- Addition of oscillation detection logic with configurable parameters for scaling factor, check depth, tolerance, and fraction thresholds
- Integration of oscillation scaling into the solver hierarchy through proper base class method calls
- Update of solver scaling methods to utilize the new oscillation detection framework
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
PhysicsSolverBase.hpp | Adds oscillation detection method declaration and solution history storage |
PhysicsSolverBase.cpp | Implements oscillation detection algorithm and integrates scaling logic into solution methods |
NonlinearSolverParameters.hpp | Defines new oscillation-related configuration parameters |
NonlinearSolverParameters.cpp | Implements parameter registration, validation, and display for oscillation settings |
CompositionalMultiphaseBase.cpp | Updates methods to call parent class for proper oscillation handling |
CompositionalMultiphaseFVM.cpp | Modifies scaling method to use base class oscillation detection |
CoupledSolver.hpp | Updates scaling method to use base class implementation |
LinearSolverParameters.cpp | Refactors table formatting for consistency |
lockExchange_rho.xml | Adds example configuration with oscillation detection enabled |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/coreComponents/physicsSolvers/NonlinearSolverParameters.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/NonlinearSolverParameters.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
an option to scale the solution changes when oscillations (on Newton iteration level) are detected:
oscillationScaling
- 0 or 1 - enables the featureoscillationScalingFactor
- between 0 and 1 - scaling factor to be applied when oscillations are detectedoscillationCheckDepth
- 2 or larger - how many iterations down the solution change history will be checkedoscillationTolerance
- check tolerance, used both to trim-out small solution changes and to check that oscillatory changes are close to each other in magnitudeoscillationFraction
- between 0 and 1 - fraction of total cells that have oscillations