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 10 commits into
base: main
Choose a base branch
from

Conversation

NishantGiridhar
Copy link

Fixes

Summary/Motivation:

In the current implementation of FlowsheetBlock, nested flowsheets with a time domain can only inherit time from the parent flowsheet.

Changes proposed in this PR:

  • Moved code to create a new time domain into a private method.
  • Create a new time domain for a nested FlowsheetBlock if time_set is provided during construction.
  • Added tests.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@NishantGiridhar NishantGiridhar added the Priority:Normal Normal Priority Issue or PR label Oct 5, 2024
@NishantGiridhar NishantGiridhar changed the title Fs nested dyn fix Fix nested flowsheet time domain construction Oct 5, 2024
Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

This looks like a good start - I only had a few minor comments.

WE should also check the documentation to make sure that what we say there is correct.

@@ -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.

# 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?

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.

idaes/core/base/tests/test_flowsheet_model.py Show resolved Hide resolved

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.

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.

@dangunter
Copy link
Member

@NishantGiridhar any update on this?

@andrewlee94
Copy link
Member

@NishantGiridhar You need to run black on the changes to get the tests to pass.

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.99%. Comparing base (96237b9) to head (1b4d02b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1495      +/-   ##
==========================================
- Coverage   76.99%   76.99%   -0.01%     
==========================================
  Files         382      382              
  Lines       61993    61997       +4     
  Branches    10146    10147       +1     
==========================================
+ Hits        47733    47734       +1     
- Misses      11852    11856       +4     
+ Partials     2408     2407       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

self.config.time = self._time
_create_time_domain()
# Creates a new time domain for a sub-flowsheet with user provided time domain
elif self.config.time_set not in [None, [0]]:
Copy link
Member

Choose a reason for hiding this comment

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

Why is the [0] here? I would have thought that if we are handed an argument we should use it. To put it another way, if the user provides [0] as an argument we will ignore it without saying why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants