Add ifail enum - #4275
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4275 +/- ##
==========================================
+ Coverage 49.24% 49.30% +0.06%
==========================================
Files 151 151
Lines 29611 29611
==========================================
+ Hits 14581 14599 +18
+ Misses 15030 15012 -18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a typed IntEnum for solver ifail exit conditions (SolverOutputCondition) and updates several runtime and test call sites to use the enum instead of hard-coded numeric values, improving readability and consistency around solver status handling.
Changes:
- Added
SolverOutputConditionenum to centralize solver output (ifail) codes. - Replaced
ifail == 1/ifail != 1checks withSolverOutputCondition.CONVERGEDin core, tracking, and vary-run utilities. - Updated regression/integration tests to assert against the enum values.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tracking/tracking_data.py | Uses SolverOutputCondition.CONVERGED for strict convergence checks when reading MFILEs. |
| tests/regression/test_process_input_files.py | Updates regression assertion to use SolverOutputCondition.CONVERGED. |
| tests/integration/test_vmcon.py | Replaces numeric ifail expectations with enum values. |
| process/data_structure/numerics.py | Adds the new SolverOutputCondition IntEnum definition. |
| process/core/solver/solver_handler.py | Uses enum values in VMCON retry logic and imports the enum. |
| process/core/scan.py | Uses enum values in optimisation/solver status reporting and VMCON error handling. |
| process/core/io/vary_run/tools.py | Uses enum values when determining feasibility/convergence from MFILE contents. |
| process/core/io/vary_run/config.py | Uses enum values to report convergence status and generate README status messages. |
| process/core/final.py | Uses enum value for final feasible/unfeasible output header selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -73,7 +74,7 @@ def run(self): | |||
| # to next attempt | |||
| class SolverOutputCondition(IntEnum): | ||
| """Enum for the possible conditions that can be returned by the solvers. | ||
| This is for the `ifail` condition | ||
| """ |
| IMPROPER_INPUT = 0 | ||
| """Solver failed due to improper input (e.g. invalid parameters, or failure to | ||
| satisfy solver preconditions)""" | ||
| CONVERGED = 1 | ||
| """Solver converged successfully""" | ||
|
|
||
| MAX_ITERATIONS = 2 | ||
| """Solver failed to converge within the maximum number of iterations""" | ||
|
|
||
| MAX_LINE_SEARCHES = 3 | ||
| """Line search required 10 function calls without finding a better solution""" | ||
|
|
||
| UPHILL_SEARCH = 4 | ||
| """Uphill search direction was calculated""" | ||
|
|
||
| NO_SOLUTION = 5 | ||
| """No feasible solution or bad approximation of Hessian""" | ||
|
|
||
| SINGLE_MATRIX_OR_BOUNDS = 6 | ||
| """Singular matrix in quadratic subproblem or restriction by artificial bounds""" |
6764908 to
6396b58
Compare
clmould
left a comment
There was a problem hiding this comment.
Some docs points:
Can the SolverOutputCondition enum be included in the vmcon page in the docs, eg something like
- a brief sentence to say that the ifail values are now contained in this enum
- an extra column in the
ifailtable to link the ifail values to the enum
Also the troubleshooting docs page mentions ifail = 3 - think it'd be worth adding a reference to the the enum for this value and adding a brief description of what this means here
| MAX_LINE_SEARCHES = 3 | ||
| """Line search required 10 function calls without finding a better solution""" | ||
|
|
||
| UPHILL_SEARCH = 4 | ||
| """Uphill search direction was calculated""" |
There was a problem hiding this comment.
Might be worth noting that these are solver specific and won't always be raised (e.g. using fsolve or some custom solver)
ace13ef to
9e8ad48
Compare
timothy-nunn
left a comment
There was a problem hiding this comment.
-
This method should be typed to return the enum
PROCESS/process/core/solver/solver.py
Line 131 in b211d2c
-
The same with the VMCON solve method. I would then also update the setting of
self.infoin this method to use the enum:
PROCESS/process/core/solver/solver.py
Line 173 in b211d2c
-
You can probably remove all references to
UPHILL_SEARCHbecause it is not used anymore as far as I can tell
| MAX_LINE_SEARCHES = 3 | ||
| # Line search required 10 function calls without finding a better solution |
There was a problem hiding this comment.
This one is solver specific, most solvers do not have an integrated line search
0f3cc80 to
e3ef760
Compare
2e74cdb to
8133217
Compare
bdd6d14 to
7233770
Compare
clmould
left a comment
There was a problem hiding this comment.
I'm happy now! Will wait for @timothy-nunn to check he is happy with his previous comments then will merge :)
timothy-nunn
left a comment
There was a problem hiding this comment.
Can we remove references to SINGULAR_MATRIX_OR_BOUNDS and UPHILL_SEARCH because:
- They are solver specific
- I don't think either have been used since the move to PyVMCON
Co-authored-by: clmould <86794332+clmould@users.noreply.github.com>
…rity and consistency
…rity and consistency
Co-authored-by: Timothy <75321887+timothy-nunn@users.noreply.github.com>
…clearer status reporting
Co-authored-by: clmould <86794332+clmould@users.noreply.github.com>
…GULAR_MATRIX_OR_BOUNDS values; update related error handling in scan.py for improved clarity.
8de68d9 to
1647e75
Compare
Description
Checklist
I confirm that I have completed the following checks: