Skip to content
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

Fix calculate_derivatives for PETSc #1342

Merged
merged 14 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 59 additions & 20 deletions idaes/core/solvers/petsc.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from pyomo.core.expr.visitor import identify_variables
import pyomo.dae as pyodae
from pyomo.common import Executable
from pyomo.dae.flatten import flatten_dae_components
from pyomo.dae.flatten import flatten_dae_components, slice_component_along_sets
from pyomo.util.subsystems import (
TemporarySubsystemManager,
create_subsystem_block,
Expand Down Expand Up @@ -428,7 +428,7 @@
symbolic_solver_labels=True,
between=None,
interpolate=True,
calculate_derivatives=True,
calculate_derivatives=False,
blnicho marked this conversation as resolved.
Show resolved Hide resolved
previous_trajectory=None,
representative_time=None,
snes_options=None,
Expand Down Expand Up @@ -711,17 +711,17 @@
# May not have trajectory from fixed variables and they
# shouldn't change anyway, so only set not fixed vars
var[t].value = vec[i]
if calculate_derivatives:
# the petsc solver interface does not currently return time
# derivatives, and if it did, they would be estimated based on a
# smaller time step. This option uses Pyomo.DAE's discretization
# equations to calculate the time derivative values
calculate_time_derivatives(m, time)
# return the solver results and trajectory if available
if calculate_derivatives:
# the petsc solver interface does not currently return time
# derivatives, and if it did, they would be estimated based on a
# smaller time step. This option uses Pyomo.DAE's discretization
# equations to calculate the time derivative values
calculate_time_derivatives(m, time, between=between)
# return the solver results and trajectory if available
return PetscDAEResults(results=res_list, trajectory=tj)


def calculate_time_derivatives(m, time):
def calculate_time_derivatives(m, time, between=None):
"""Calculate the derivative values from the discretization equations.

Args:
Expand All @@ -731,19 +731,58 @@
Returns:
None
"""
# Leave between an optional argument for backwards compatibility
if between is None:
between = time
for var in m.component_objects(pyo.Var):
if isinstance(var, pyodae.DerivativeVar):
if time in ComponentSet(var.get_continuousset_list()):
parent = var.parent_block()
name = var.local_name + "_disc_eq"
disc_eq = getattr(parent, name)
for i, v in var.items():
try:
if disc_eq[i].active:
v.value = 0 # Make sure there is a value
calculate_variable_from_constraint(v, disc_eq[i])
except KeyError:
pass # discretization equation may not exist at first time
parent_block = var.parent_block()
disc_eq = getattr(parent_block, var.local_name + "_disc_eq")

deriv_dict = dict(
(key, pyo.Reference(slc))
for key, slc in slice_component_along_sets(var, (time,))
)
disc_dict = dict(
(key, pyo.Reference(slc))
for key, slc in slice_component_along_sets(disc_eq, (time,))
)

for key, deriv in deriv_dict.items():
# state = state_dict[key]
disc_eq = disc_dict[key]
for t in time:
if t < between.first() or t > between.last():
# Outside of integration range, skip calculation
continue
old_value = deriv[t].value
try:
# TODO This calculates the value of the derivative even
# if one of the state var values is from outside the
# integration range, so long as it's initialized. Is
# this the desired behavior?
if disc_eq[t].active and not deriv[t].fixed:
deriv[t].value = 0 # Make sure there is a value
calculate_variable_from_constraint(deriv[t], disc_eq[t])
except KeyError as err:
# Discretization and continuity equations may or may not exist at the first or last time
# points depending on the method. Backwards skips first, forwards skips last, central skips
# both (which means the user needs to provide additional equations)
if t == time.first() or t == time.last():
pass
else:
raise err

Check warning on line 775 in idaes/core/solvers/petsc.py

View check run for this annotation

Codecov / codecov/patch

idaes/core/solvers/petsc.py#L775

Added line #L775 was not covered by tests
except ValueError as err:
# At edges of between, it's unclear which adjacent
# values of state variables have been populated.
# Therefore we might get hit with value errors.
if t == between.first() or t == between.last():
# Reset deriv value to old value
if disc_eq[t].active and not deriv[t].fixed:
deriv[t].value = old_value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbianchi-lbl Do you know what Pylint is complaining about here? We can only get to this logic if old_value has already been assigned in the try block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick look I'd guess it might be because of old_value is defined inside the if block on L764?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it outside the if block, still getting the pylint error. I'm guessing it's because Pylint doesn't know that accessing deriv[t].value is only going to raise a KeyError in that context, and will never raise a ValueError. I assume this is a case for some Pylint ignore statement.

The test failures appear to be unrelated to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it might work if old_value = deriv[t].value is placed at the top of the for t in time: loop block, i.e. on line 756 (before the if t < between.first() ... line)?

If that doesn't work either, I agree that a Pylint disable directive seems warranted.

else:
raise err

Check warning on line 785 in idaes/core/solvers/petsc.py

View check run for this annotation

Codecov / codecov/patch

idaes/core/solvers/petsc.py#L785

Added line #L785 was not covered by tests


class PetscTrajectory(object):
Expand Down
Loading
Loading