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

Calculate state method #178

Merged
merged 22 commits into from
Aug 9, 2021
Merged

Conversation

TimBartholomew
Copy link
Contributor

Fixes/Addresses:

Fixes #57

Summary/Motivation:

Oftentimes users want to fix properties on a stateblock and while this is possible it frequently causes errors in initialization. This happens because stateblock initialization routines assume the state variables should be fixed before the solve call. This PR creates another method called calculate_state, where a stateblock is solved for user provided variables and values (that can be properties).

Changes proposed in this PR:

  • Adds calculate state method to the NaCl and Seawater property models
  • Adds testing for the new method

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.

@TimBartholomew TimBartholomew self-assigned this Jul 23, 2021
@TimBartholomew TimBartholomew added Priority:High High Priority Issue or PR model dev team Issues directly related to the model development team labels Jul 23, 2021
@andrewlee94
Copy link
Collaborator

@TimBartholomew What is the actual use case for this method, and how do you see it being used? I can see some potential issues with this, and it would help to know exactly what you intend this for. I will also note that as it stands this would only affect the ProteusLib specific property packages, so would hardly be general, which might be confusing to some users.

Also, will ask the question of how this would differ from initializing the properties at some known state (hopefully close to the final requirement), and then fixing/specifying the required properties after initialization. For example, this is how the isentropic pressure changer works (and actually even the energy balances). We start by initializng the isentropic state at a condition close to the final solution, and then add a constraints that fix pressure and entropy for the actual solve.

def test_calculate_state(self, frame_stateblock):
m = frame_stateblock

# check if calculate_state runs and solves rather than how its used
Copy link
Contributor

@adam-a-a adam-a-a Jul 23, 2021

Choose a reason for hiding this comment

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

Can you add a test that demonstrates how supplying volumetric feed flow rate and mass concentration to calculate_state would lead to some expected set of results?

After all, in my view, that is the main point for introducing this method--we want to be able to specify flow_vol_phase and conc_mass_phase_comp when fixing variables instead of being restricted to fixing component mass flow rates only, which is less straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I haven't finished the PR yet. I'm planning on adding an individual test class in the property test harness for calculate_state cases. I'm also going to use it in the RO with energy recovery flowsheet.

@TimBartholomew
Copy link
Contributor Author

@TimBartholomew What is the actual use case for this method, and how do you see it being used? I can see some potential issues with this, and it would help to know exactly what you intend this for. I will also note that as it stands this would only affect the ProteusLib specific property packages, so would hardly be general, which might be confusing to some users.

Also, will ask the question of how this would differ from initializing the properties at some known state (hopefully close to the final requirement), and then fixing/specifying the required properties after initialization. For example, this is how the isentropic pressure changer works (and actually even the energy balances). We start by initializng the isentropic state at a condition close to the final solution, and then add a constraints that fix pressure and entropy for the actual solve.

@andrewlee94 I see this method being used before initialization routines in a variety of cases.

  1. Determining and setting inlet state variables when the user prefers to provide properties. For example, multiple proteuslib developers prefer to set up the inlet conditions with a flowrate and concentration rather than component mass flowrates. With this method then they can calculate the state with hold_state as true, and then use the initialization method and propagate states as is.
  2. Determining state variables within system-wide initialization routines. Similar to 1, there are points within the initialization of a system that uses a pressure exchanger where I want to base a split fraction based on volumetric flowrate rather than state variables. With how our initialization tools are built though I need to calculate the state variables in order to propagate the state. In that routine, I fix the properties on a state block, check the degrees of freedom, solve the block, check the solver status, and unfix the properties. This method does that all in one method call and includes descriptive exceptions for the checks.
  3. Determining state variables within a unit initialization method. There may be cases where initializations could be improved if state blocks were solved for some property.

Cases 1 and 2 will be demonstrated in this PR soon for the RO with energy recovery flowsheet, this was still a work in progress.

@TimBartholomew TimBartholomew marked this pull request as draft July 26, 2021 19:40
@andrewlee94
Copy link
Collaborator

andrewlee94 commented Jul 26, 2021

@TimBartholomew So, the first thing I can see is that a tool like this is going to be very property-package specific - i.e. I do not think it will be possible to build a generalised version of this. This means that any implementation is going to be case-specific, and it does mean that users might be confused when only certain property packages support this behaviour (and only for certain properties).

There may still be value in doing this, but I do see a lot of caveats and edge cases that will arise from this (and I do mean will). To put somethings in writing and allow some thought/discussion (note that these are written from a general stand point, many of these could be trivial for specific examples):

  1. This will only work if the properties the user desired to fix are Vars - it will not work if they are written as Expressions (e.g. the generic property framework). So, what properties this can be used on will depend on the property package in question and how it is written.
  2. Trying to use this on an uninitialized property package could be (will be?) hit-or-miss, as intermediate variables may not be available. E.g. using concentration in place of mole fractions requires solving for density. For simpler property packages this might work, but it could easily become challenging for more complex property packages. The practical definition of state variables is that all other properties are explicit in terms of these. The reverse means that state variables are generally implicit in terms of other properties (if you have only one "other" property then you can back-solve for the missing state, but this is a very limited use case, and is still not necessarily simple).
  3. Degrees of freedom could be challenging, and users could easily specify a degenerate set of conditions if they are not careful. This is especially true for inlets where there is one less degree of freedom due to sum of mole fraction constraints (or equivalent).
  4. You are starting to move towards a combinatorial problem. The number of potential property specifications a user could "know" is huge, and each set of combinations require a different approach to resolve to a solvable state. Obviously, we cannot support a truly arbitrary set of specifications, so any implementation begins to become very case-specific.

In terms of the specific case studies you mentioned, I would suggest that in all cases you would get more reliable results by initializing the state block first at your best current guess (even if that is the inlet conditions), and then switching to your preferred definition of the system. Yes, it is one more step, but it avoids a lot of potential problems as the model constraints are generally written explicitly in terms of the state variables (that is what makes them state variables). Otherwise, you are now trying to solve a model based on implicit relationships which is much harder.

As I mentioned before, this is how the isentropic pressure changer model works. The isentropic state should be solved based on the inlet flowrate and composition, inlet specific entropy and outlet pressure. However, specific entropy is very rarely a state variable, so instead we initialize at the inlet flow, composition and temperature and outlet pressure. This is not correct, but is generally very close enough such that we can then just solve the entire unit with the isentropic constraint. A similar argument holds for the actual outlet condition (where the outlet specific enthalpy, pressure, flow and composition is known).

Of your listed use cases, case 1 is probably the most important as it is often the case that the user knows the inlet in one set of properties, but the calculations are better performed using a different set of state variables. However, even here my gut feeling is that it would be better to initialize the state block using the best guesses of the actual state variables, and to only apply the desired specification once the state is initialised. In terms of case 2 and 3, I very much think that is the case - just initialize using simple guesses based on the state variables and then apply the actual spec. Unless you have some really weird system, I cannot see your best guess being too far off the mark.

TLDR: It is almost always easier to initialize at a set of initial guesses for the state variable first and get a feasible solution which is close to your desired conditions, and to then move to the actual state you want rather than to try to directly solve for the condition you want by specifying properties derived from the state variables.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #178 (3d58b4d) into main (b228354) will decrease coverage by 0.00%.
The diff coverage is 82.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   90.11%   90.10%   -0.01%     
==========================================
  Files          21       21              
  Lines        3611     3649      +38     
==========================================
+ Hits         3254     3288      +34     
- Misses        357      361       +4     
Impacted Files Coverage Δ
...ith_energy_recovery/monte_carlo_sampling_RO_ERD.py 95.23% <ø> (ø)
proteuslib/property_models/seawater_prop_pack.py 98.07% <78.12%> (-0.65%) ⬇️
proteuslib/property_models/NaCl_prop_pack.py 97.61% <78.78%> (-0.54%) ⬇️
...RO_with_energy_recovery/RO_with_energy_recovery.py 89.71% <100.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b228354...3d58b4d. Read the comment docs.

@TimBartholomew TimBartholomew marked this pull request as ready for review August 3, 2021 18:24
@TimBartholomew
Copy link
Contributor Author

@adam-a-a @bknueven @lbianchi-lbl have we seen these test failures before?

  1. py3.9/linux is failing in doc link checking, but I haven't touched any doc files.

  2. py3.7/linux passed when I did my latest commit, but after I reran the tests trying to see if the above failure was a fluke, then this failed at installing the dependencies.

adam-a-a
adam-a-a previously approved these changes Aug 5, 2021
Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

Despite our collective comments related to the API discussion, I think all of that can be saved for another issue/PR. This looks good to me.

self.solver = 'ipopt'
self.optarg = {'nlp_scaling_method': 'user-scaling'}

self.scaling_args = {('flow_mass_phase_comp', ('Liq', 'H2O')): 1e-1,
Copy link
Contributor

@adam-a-a adam-a-a Aug 5, 2021

Choose a reason for hiding this comment

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

EDIT: I only noticed the full API discussion after writing this. No need to respond.
This would be tedious, would apply to all dictionaries related to PropertyTestHarness objects, and would be for a future PR, but I think we may want to consider reformatting how the variable dicts are entered. Unless there's a reason for why things were written this way, I think we should change it to be more consistent with the way things are usually written in IDAES/Proteuslib. (@TimBartholomew @andrewlee94 Please correct me if I am wrong.)

For example, instead of
('flow_mass_phase_comp', ('Liq', 'H2O')): 1e-1
replace with
{'flow_mass_phase_comp[Liq, 'H2O']: 1e-1

The difference is subtle, but I could see future developers/users getting confused between the different formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adam-a-a so you are advocating for form 4 described above, right?

adam-a-a
adam-a-a previously approved these changes Aug 5, 2021
bknueven
bknueven previously approved these changes Aug 6, 2021
@adam-a-a adam-a-a dismissed stale reviews from bknueven and themself via 3d58b4d August 9, 2021 17:33
@adam-a-a adam-a-a enabled auto-merge (squash) August 9, 2021 17:48
@adam-a-a adam-a-a merged commit 435ba10 into watertap-org:main Aug 9, 2021
@TimBartholomew TimBartholomew deleted the calculate_state branch April 29, 2022 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
model dev team Issues directly related to the model development team Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add calculate_state method to property packages
6 participants