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 nested flowsheet time domain construction #1495

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
60 changes: 37 additions & 23 deletions idaes/core/base/flowsheet_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,31 @@ def _get_stream_table_contents(self, time_point=0):
return self.stream_table(time_point)

def _setup_dynamics(self):
def _create_time_domain():
if self.config.dynamic:
# Check if time_set has at least two points
if len(self.config.time_set) < 2:
# Check if time_set is at default value
if self.config.time_set == [0.0]:
# If default, set default end point to be 1.0
self.config.time_set = [0.0, 1.0]
else:
# Invalid user input, raise Exception
raise DynamicError(
"Flowsheet provided with invalid "
"time_set attribute - must have at "
"least two values (start and end)."
)
# For dynamics, need a ContinuousSet
self._time = ContinuousSet(initialize=self.config.time_set)
else:
# For steady-state, use an ordered Set
self._time = pe.Set(initialize=self.config.time_set, ordered=True)
self._time_units = self.config.time_units

# Set time config argument as reference to time domain
self.config.time = self._time

# Look for parent flowsheet
fs = self.flowsheet()

Expand Down Expand Up @@ -342,35 +367,24 @@ def _setup_dynamics(self):
"{} was set as a dynamic flowsheet, but time domain "
"provided was not a ContinuousSet.".format(self.name)
)

if self.config.dynamic is False and not isinstance(
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is this check actually necessary? This would appear to preclude putting steady-state flowsheets inside dynamic flowsheets.
  2. if self.config.dynamic is False can be written as if not self.config.dynamic

Copy link
Author

Choose a reason for hiding this comment

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

That's exactly what it was checking for. Since we don't want to allow adding a nested dynamic flowsheet inside a parent steady-state flowsheet, I will remove this check from this PR.

self.config.time, pe.Set
):
raise DynamicError(
"{} was set as a steady-state flowsheet, but time domain "
"provided was not a Set.".format(self.name)
)
add_object_reference(self, "_time", self.config.time)
self._time_units = self.config.time_units
else:
# If no parent flowsheet, set up time domain
if fs is None:
# Create time domain
if self.config.dynamic:
# Check if time_set has at least two points
if len(self.config.time_set) < 2:
# Check if time_set is at default value
if self.config.time_set == [0.0]:
# If default, set default end point to be 1.0
self.config.time_set = [0.0, 1.0]
else:
# Invalid user input, raise Exception
raise DynamicError(
"Flowsheet provided with invalid "
"time_set attribute - must have at "
"least two values (start and end)."
)
# For dynamics, need a ContinuousSet
self._time = ContinuousSet(initialize=self.config.time_set)
else:
# For steady-state, use an ordered Set
self._time = pe.Set(initialize=self.config.time_set, ordered=True)
self._time_units = self.config.time_units

# Set time config argument as reference to time domain
self.config.time = self._time
_create_time_domain()
elif self.config.time_set is not None:
Copy link
Member

Choose a reason for hiding this comment

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

I think this is enough, but just to check is there any additional validation we should do here? Also, an in-line comment to explain what this case is (sub-flowsheet with user-provided time domain).

Copy link
Author

Choose a reason for hiding this comment

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

Based on my testing I think this should be good enough. The _create_time_domain method makes sure that the specified time_set list is of the right length. So the same checks that apply for time domain construction in a parent flowsheet should apply to the nested flowsheet.

@dallan-keylogic could Are there any edge cases I am overlooking?

# Create time domain from config.time_set
_create_time_domain()
else:
# Set time config argument to parent time
self.config.time = fs.time
Expand Down
33 changes: 33 additions & 0 deletions idaes/core/base/tests/test_flowsheet_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,39 @@ def test_dynamic_external_time(self):
assert m.fs.sub.config.time is m.s
assert m.fs.sub.time_units == units.minute

@pytest.mark.unit
def test_dynamic_nested_time(self):
m = ConcreteModel()
m.s1 = ContinuousSet(initialize=[4, 5])
andrewlee94 marked this conversation as resolved.
Show resolved Hide resolved
m.s2 = ContinuousSet(initialize=[1,2,3])
m.fs = FlowsheetBlock(dynamic=True, time_units=units.s)
m.fs.sub = FlowsheetBlock(dynamic=True, time=m.s2, time_units=units.s)

assert m.fs.sub.config.dynamic is True
assert isinstance(m.fs.sub.time, ContinuousSet)
Copy link
Member

Choose a reason for hiding this comment

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

Here you should do assert m.fs.time is m.s2. - i.e. check that the time domain is the correct set, not just the correct type.

assert m.fs.sub.time_units == units.s

@pytest.mark.unit
def test_dynamic_nested_time_from_time_set(self):
m = ConcreteModel()
m.fs = FlowsheetBlock(dynamic=True, time_set = [1,2,3], time_units=units.s)
m.fs.sub = FlowsheetBlock(dynamic=True, time_set = [1, 2], time_units=units.s)

assert m.fs.sub.config.dynamic is True
assert isinstance(m.fs.sub.time, ContinuousSet)
assert m.fs.sub.time_units is units.s
andrewlee94 marked this conversation as resolved.
Show resolved Hide resolved

@pytest.mark.unit
def test_dynamic_parent_time_indexed_ss_child(self):
m = ConcreteModel()
m.fs = FlowsheetBlock(dynamic=True, time_set = [1,2,3], time_units=units.s)
m.fs.sub = FlowsheetBlock(dynamic=False, time_set = [0, 1], time_units=units.dimensionless)
Copy link
Member

@andrewlee94 andrewlee94 Oct 9, 2024

Choose a reason for hiding this comment

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

Time should not be dimensionless - it does not matter for the test, but we should not accidentally encourage people to think this is OK.


assert m.fs.sub.config.dynamic is False
assert isinstance(m.fs.sub.time, Set)
assert m.fs.sub.time_units == units.dimensionless
Copy link
Member

Choose a reason for hiding this comment

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

As above.



@pytest.mark.unit
def test_dynamic_external_time_invalid(self):
m = ConcreteModel()
Expand Down
Loading