-
Notifications
You must be signed in to change notification settings - Fork 36
Conversation
@jghouse88, can you take a look? |
@andrewlee94 I have reviewed this and made some changes to the text and added a testing cell. I would like you to go over the description especially the sections that describe the idaes components required for building the config dict. |
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
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 think we need to think a little bit more about the audience for this example and provide more description. This is basically the first introduction to the generic property framework, so I think the first section needs to explain what the generic framework is and what it does - i.e. part 1 should be more what and why rather than how. Part 2 can then be the how, with an example of a specific system (CO2-H2O), and Part 3 then being a demonstration of how to put all of this to use in a flowsheet.
@@ -129,6 +129,7 @@ contents: | |||
desc: Custom property packages | |||
notebooks: | |||
- air_separation_case_study: "Property packages for air separation units (N2, O2, Ar mixture)" | |||
- ideal_h2o_co2_vle_prop_pack: "Property package for ideal water-carbondioxide mixture" |
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.
Typo: space missing between carbon dioxide
"source": [ | ||
"### Import the required Python and Pyomo packages\n", | ||
"\n", | ||
"1. 'logging' is a python package required to set up the logger\n", |
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.
logging is not imported in the next cell.
"\n", | ||
"IDAES components required for building the property package need to be imported. They are as follows:\n", | ||
"\n", | ||
"1. 'LiquidPhase', 'VaporPhase', 'PhaseType', are classes that are required to declare phase types for components\n", |
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.
PhaseType is an Enum and is used to identify the type of a phase (rather than to declare a phase).
"\n", | ||
"1. 'LiquidPhase', 'VaporPhase', 'PhaseType', are classes that are required to declare phase types for components\n", | ||
"\n", | ||
"2. 'Component' is the class required to declare different system constituents as components in the system\n", |
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.
In all points, I would use "a" rather than "the" - "the" indicates that there is only a single possibility, whilst "a" indicates that there can be alternatives.
"source": [ | ||
"A nested dictionary needs to be created in order to specify the components involved in the system, their thermophysical and thermodynamic property corelations, corresponding parameters, and valid phases. Along with this, it is also meant to provide system level details, like the combination of state variables for state definition, valid bounds for each of them, reference state, and phases in equilibrium. \n", | ||
"\n", | ||
"The configuration dictionary has two main types of keys:\n", |
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.
This is not correct, as a number of things can have nested information (e.g. Phases).
Rather I would just say that the information starts with the following system wide information, and that each key can contain further more specific information.
It should also be noted here that not all of these a re required for all applications - e.g. single phase systems can skip the phase equilibrium information. We should probably note those keys that are always required (components, phases, base_units, state_definition, temperature_ref and pressure_ref), and then mention that there are other optional keys and point to the docs (as there are some that you have missed here).
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"This key contains a list of tuples as value, each tuple containing names of phases in equilibrium for the system" |
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 need to explain exactly what this does and why it is needed. We should also warn that you cannot have cyclic-systems (e.g. A <-> B <-> C <->A) as this would create a degenerate system.
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"This key contains a dictionary as value, with the method required from idaes core library to obtain a smooth formulation for phase equilibrium equations" |
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.
This needs to be explained better.
Phase equilibrium is an inherently discontinuous phenomena; phase may appear or disappear depending on the state of the system. This creates problems for equation-oriented modeling, as it is assumed that there is always a feasible solution for the phase equilibrium constraints. One approach for addressing this is to solve the phase equilibrium problem at a separate "phase equilibrium state" where both phases exists. This "phase equilibrium state" should be selected such that it is identical to the actual system state if the system is in the two-phase region, or equal to the bubble or dew point if the system is outside the two-phase region. This key is used to select a method to use for determining the "phase equilibrium state" for this property package.
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"This key contains the method required from idaes core library to calculate bubble and dew points for the system, as a value" |
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.
As is apparent from the discussion above, the calculation of bubble and dew points is important for solving phase equilibrium problems. A universal set of calculations exists for determining bubble and dew points that is applicable to all equations of state and similar approaches, however this can be greatly simplified if the system follows ideal gas and liquid behaviors (i.e. Raoult's Law). This key allows users to select from either the general set of equations or the simplified ideal equations for calculating bubble and dew points in this property package. Note that the ideal equations should not be used if wither phase shows non-ideal behavior.
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"configuration = {\n", |
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 think we need more description here. Given that this is basically a tutorial, I think the first part should provide a general description of the overall tool and what each part does and why you want to set it. The second part (i.e. here) should then show how to set each part for a given application with some justification/discussion of why/how each value was chosen.
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"## Part 2: Using the property package with a Flash unit model" |
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 think a little bit of description is needed here, but not much.
E.g. this part should say that we will now demonstrate how to use the property package in a flowsheet and solve a simple test problem.
@anujad95, any update on this? |
@anujad95 what about this one? |
@anujad95 is this still possible for the Aug release? |
@ksbeattie I will try to get through this as soon as possible. |
@anujad95, moving this off of the Nov release to the Feb release. Please let me know if that shouldn't be done. |
@anujad95, this is now over a year old, so I'm thinking we close this and let you open it again when you can. |
@anujad95, I'm going to close this. Please re-open or re-create if/when you get back to it. |
Fixes # .
NA
Proposed changes:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: