idaes-pse icon indicating copy to clipboard operation
idaes-pse copied to clipboard

Complementarity-based VLE formulation

Open viiibhav opened this issue 2 years ago • 4 comments

Fixes

Summary/Motivation:

This pull request seeks to add the new complementarity-based vapor-liquid equilibrium formulation with thermodynamic models supported by cubic equations of state. Currently, only the subcritical regime is activated.

Changes proposed in this PR:

  • Addition of the new formulation and modifying the scripts generic_property.py, smooth_VLE.py and ceos.py
  • Addition of critical volume and critical compressibility factor in the property packages BT_PR.py and ASU_PR.py
  • Addition of basic unit tests in test_flash_PR.py

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.

viiibhav avatar Oct 03 '22 18:10 viiibhav

A few general comments to get things started:

1. You need to run `black` on the code before committing, otherwise tests will not get run. I also noted that the branch is out of date with the current main.

2. Whilst I understand that it is easier to implement in parallel (rather than trying to update the existing code in one go), we should not be committing both versions side-by-side like this (especially not in the "core" parts of IDAES where we want to promise backwards compatibility). For now we should probably work on a branch whilst we refine everything.

3. This will also make reviewing the code a lot easier - as the files are currently all "new", it is hard to tell what you have changed here, and what is just copied from the original.

We deliberately decided not to modify existing code but take it in steps. What do you suggest we do?

jghouse88 avatar Oct 04 '22 14:10 jghouse88

@jghouse88 Working on a branch is probably best for now - the alternative would be to put it in models_extra but I am not sure that would be any better. As I noted, a branch will also make it clearer what changes are being made to the files as we can easily see the diff.

However, we definitely should not be merging this into models in the current state.

EDIT: Also, on a branch we can allow tests to fail in the interim period (if required), so we can work directly on the main files and use the test failure to track any issues that still need to be addressed.

andrewlee94 avatar Oct 04 '22 14:10 andrewlee94

@andrewlee94 We ideally want to deprecate the older smooth VLE formulation that uses the bubble/dew point. I would rather this go to the models folder than models extra and it languishing there without getting used. What would be necessary in addition to what @viiibhav has done here to get to models?

jghouse88 avatar Oct 07 '22 15:10 jghouse88

@viiibhav You and I will need to redo this PR to show what changes are happening from the previous version of the CEOS which means you will need to make changes directly to those files and not create a copy with _ncp tag.

jghouse88 avatar Oct 07 '22 15:10 jghouse88

@viiibhav Also note you have some conflicts to fix (which you should probably do first before they get any worse). These should all be due to some changes I made in how the metadata for units are defined - before they were stored as a dict (derived_units["temperature"]) but now they exist as @properties (derived_units.TEMPERATURE, which supports auto-completion). note that the old approach still works for backward compatibility, but the new approach is preferred.

Also be aware of PR #995, which does a similar thing for thermophsyical properties as well (although there is still a fair bit of work left for that PR to be ready).

andrewlee94 avatar Oct 31 '22 11:10 andrewlee94

@viiibhav any updates on this and/or resolving these conflicts?

ksbeattie avatar Dec 15 '22 19:12 ksbeattie

@ksbeattie I will be pushing some more updates soon.

viiibhav avatar Dec 15 '22 19:12 viiibhav

@viiibhav is this something that will be ready for the Feb release, meaning ready to merge in the next 2 weeks?

ksbeattie avatar Feb 02 '23 19:02 ksbeattie

@ksbeattie probably not, but I will try and get to it.

viiibhav avatar Feb 02 '23 19:02 viiibhav

@viiibhav is this now possible for the May release?

ksbeattie avatar Mar 09 '23 19:03 ksbeattie

@ksbeattie yes it should be ready by then.

viiibhav avatar Mar 16 '23 18:03 viiibhav

@viiibhav any news on this?

ksbeattie avatar Apr 27 '23 18:04 ksbeattie

@ksbeattie Still working on it, was caught up with defense and graduation.

viiibhav avatar May 01 '23 03:05 viiibhav

Moving this to the Aug release, in hopes it might get done by then.

ksbeattie avatar May 18 '23 18:05 ksbeattie

No news as whether this is being worked on for the Aug release.

lbianchi-lbl avatar Jun 29 '23 18:06 lbianchi-lbl

@viiibhav let us know if there are any updates on whether or when you'll be able to resume work on this.

lbianchi-lbl avatar Jul 13 '23 18:07 lbianchi-lbl

@viiibhav closing this due to inactivity.

ksbeattie avatar Jul 27 '23 18:07 ksbeattie