-
Notifications
You must be signed in to change notification settings - Fork 234
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
PETSc Tweaks #1235
PETSc Tweaks #1235
Conversation
@@ -420,8 +420,8 @@ def petsc_dae_by_time_element( | |||
initial_variables=None, | |||
detect_initial=True, | |||
skip_initial=False, | |||
snes_solver="petsc_snes", | |||
snes_options=None, | |||
initial_solver="petsc_snes", |
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.
Seeing as we are pushing to get backward compatibility tests into place, this look like a place where we are going to need backward compatibility options and warnings. I.e. we need to keep the old options around, and have them raise deprecation warnings if they are used before sending the value to the new argument (and raise an exception if both old and new are provided).
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 was hoping that this feature being around for only a month and not making it into a major release would prevent that from being necessary. Perhaps not, however.
Edit: snes_solver
has been around only for a month. However, snes_options
has been around longer.
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.
If it is that new then we are probably OK, but we need to double check that is the 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.
Got the deprecation logged for snes_options
. Ready for a new review.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1235 +/- ##
=======================================
Coverage 76.71% 76.71%
=======================================
Files 382 382
Lines 61182 61188 +6
Branches 11289 11291 +2
=======================================
+ Hits 46935 46940 +5
- Misses 11814 11817 +3
+ Partials 2433 2431 -2
☔ View full report in Codecov by Sentry. |
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 wanted to make this change when I added the snes_solver option, since it's not necessarily an SNES solver anymore, but I didn't want to break anything. Once @andrewlee94 gets his deprecation warning, I'm good with this.
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.
Looking good - just a couple of minor comments to avoid some edge cases and be more inline with other deprecations.
idaes/core/solvers/petsc.py
Outdated
regular_vars, time_vars = flatten_dae_components(m, time, pyo.Var) | ||
regular_cons, time_cons = flatten_dae_components(m, time, pyo.Constraint) | ||
if snes_options is not None: | ||
logger = idaeslog.getIdaesLogger("idaes") |
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.
We generally use logger = idaeslog.getLogger(__name__)
for this - using __name__
gives more granularity and avoids hard-coded string constants. Also, Pyomo has a deprecation warning function for things like this:
from pyomo.common.deprecation import deprecation_warning
deprecation_warning(
msg=msg, logger=logger, version="2.0.0", remove_in="3.0.0"
)
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.
LGTM
Changes proposed in this PR:
-Only include active blocks when constraints/variables are collected for PETSc solve
-Rename
snes_solver
option toinitial_solver
because the point of the option is to permit use of initial solvers besides snes-Add symbolic solver labels to initial condition problem
-Get rid of unused list
fix_derivs
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: