Fix bubblepoint function and unit inconsistency in OLI API #1388

adam-a-a · 2024-05-14T16:22:21Z

Fixes/Resolves:

Fixes problem that prevents successfully calling bubblepoint function to get saturation pressure or temperature
also fixes Updating units in OLI Flash #1369

Summary/Motivation:

In an attempt to call the bubblepoint function, I discovered that it doesn't work as intended and the user would not be able to run the function. Along the way, I also noticed that input and/or output units provided for OLI properties have oli_unit and pyomo_unit attributes, and when one sets one or the other, there would be inconsistencies between the two (see #1369).

Changes proposed in this PR:

add vapor phase as requirement for bubblepoint func to run
add bubblepoint to list of post flash methods that are available
deal with flatten_results for unanticipated vaporDiffusivityMatrix, which breaks the logic of flatten_results
refine FixedKeysDict so that when the user sets either pyomo_unit or oli_unit, the other will automatically update via __setitem__ and using nested FixedKeysDicts
add tests for new FixedKeysDict functionality
add test for bubblepoint
add test for valid_phases

Legal Acknowledgement

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

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

…itymatrix results; reminder to check diff and remove debugging bits and finalized fixedkeysdict changes

… bubblepoint; test fixedkeysdict funcitonality with resolved units issue

watertap/tools/oli_api/flash.py

codecov · 2024-05-14T17:40:26Z

Codecov Report

Attention: Patch coverage is 83.82353% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 95.23%. Comparing base (21c209b) to head (b5bcc5c).

Files	Patch %	Lines
watertap/tools/oli_api/util/fixed_keys_dict.py	80.00%	7 Missing ⚠️
watertap/tools/oli_api/flash.py	77.77%	2 Missing ⚠️
watertap/tools/oli_api/client.py	88.88%	1 Missing ⚠️
watertap/tools/oli_api/conftest.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1388      +/-   ##
==========================================
- Coverage   95.25%   95.23%   -0.02%     
==========================================
  Files         335      335              
  Lines       35727    35621     -106     
==========================================
- Hits        34033    33925     -108     
- Misses       1694     1696       +2

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

bknueven

I am not sure about the automatic units conversion piece of this.

In the original conception, I imaged using the "pyomo_unit" attribute so the user could convert whatever input they had to units which will work with OLI by using the existing units conversion in Pyomo, and then back to those units after OLI returns the data.

It could be the answer is to restructure this so that the "pyomo_unit" and "oli_unit" are read-only attributes so as not to confuse the user.

bknueven · 2024-05-15T21:03:56Z

watertap/tools/oli_api/util/fixed_keys_dict.py

@@ -25,7 +27,48 @@ def __setitem__(self, k, v):
            raise RuntimeError(f" Key {k} not in dictionary.")
        # also check for valid value if a list of values is given in the default dictionary
        else:
-            self.data[k] = v
+            # if user setting value in pyomo units


Maybe we should have a sub-class of FixedKeysDict just for the units?

bknueven · 2024-05-15T21:06:47Z

watertap/tools/oli_api/util/fixed_keys_dict.py

+        "oli_unit": "Pa-s",
+        "pyomo_unit": pyunits.Pa * pyunits.second,
+    },
+    "alkalinity": {"oli_unit": "mg HCO3/L", "pyomo_unit": pyunits.mg / pyunits.L},


This example, among others, is why Paul and I gave up on an automatic approach. It seemed like you could be working forever on every edge case and still miss some.

Right- we'd need to set valid units accepted for each property, which is documented. We may catch bugs along the way (i.e., what's documented may not match reality in some cases).

adam-a-a · 2024-05-16T00:50:54Z

So, I am not completely sure about the utility of pyomo_unit since we technically haven't applied it yet. Nevertheless, assuming that it might come in handy later and aiming to maintain what you guys put together, I decided to implement in this fashion, primarily because the immediate priority is to allow for the user to be able change units for inputs/outputs readily. I suppose this wasn't the original intent for FixedKeysDict, but that is what is most important to start with. These changes should make that possible. I think we can decide on totally reworking this later, but for now, i want to use it for a subsequent PR that will involve the need to toggle units of inputs and outputs.

bknueven · 2024-05-16T01:14:32Z

Maybe "pyomo unit" should be replaced with "pint unit".

The idea of the existing mapping is if the user wants to change the units on the input / output they can do that in a standard way through Python, instead of needing to deal with the units in OLI at all.

adam-a-a · 2024-05-16T01:21:47Z

Maybe "pyomo unit" should be replaced with "pint unit".

The idea of the existing mapping is if the user wants to change the units on the input / output they can do that in a standard way through Python, instead of needing to deal with the units in OLI at all.

Ah, so say I want to alter the units for "molecularConcentration" to mol/L, which happens to be set to mg/L by default. How would you imagine that would go? Would it be something like the following? Or do you have a different example in mind?

f = Flash()
f.input_unit_set['molecularConcentration']['pint_unit'] = pyunits.mol/pyunits.L

bknueven · 2024-05-16T01:39:42Z

Ah, so say I want to alter the units for "molecularConcentration" to mol/L, which happens to be set to mg/L by default. How would you imagine that would go? Would it be something like the following? Or do you have a different example in mind?

So, for this particular issue, I would suggest we mirror what we already do in other parts of IDAES / WaterTAP: force the user to tell us up-front if they want an molar or mass basis and select the appropriate mapping for them.

…eneration

…into fix_oli_client

adam-a-a · 2024-05-16T16:40:57Z

Ah, so say I want to alter the units for "molecularConcentration" to mol/L, which happens to be set to mg/L by default. How would you imagine that would go? Would it be something like the following? Or do you have a different example in mind?

So, for this particular issue, I would suggest we mirror what we already do in other parts of IDAES / WaterTAP: force the user to tell us up-front if they want an molar or mass basis and select the appropriate mapping for them.

That could be problematic given the way properties are passed/returned in OLI. We can think about changing this entirely. In any case, the current code resolved the issue that I identified. Now, if you overwrite either oli_unit or pyomo_unit with string or pint unit (all options are compatible), pyomo_unit would automatically match the newly set oli_unit; conversely, setting the pyomo_unit to a new value (string or pint units), would automatically update the corresponding oli_unit.

I think we can decide to accept as an incremental change and revisit in subsequent PRs, or I can strip that segment from this PR. If I do the latter, I will likely reintroduce those mods in a subsequent PR which would illustrate an example of why the user might need to change units for certain workflows in a non-obvious way. In the longer-term, we can think about a total refactor of the related code to better match practical user needs. Most importantly, I want to have some treatment of this before it ends up in 1.0.

bknueven · 2024-05-16T16:54:33Z

Now, if you overwrite either oli_unit or pyomo_unit with string or pint unit (all options are compatible), pyomo_unit would automatically match the newly set oli_unit; conversely, setting the pyomo_unit to a new value (string or pint units), would automatically update the corresponding oli_unit.

My issue is that this does not work for all cases because the OLI units are bespoke, e.g., #1388 (comment), and that issue aside, according to the documentation OLI will only take a fixed set of units for each parameter.

Because of this, I would suggest if the user wants different units to go to OLI we make them specify them as OLI unit / pint unit pair.

adam-a-a · 2024-05-16T17:23:12Z

Now, if you overwrite either oli_unit or pyomo_unit with string or pint unit (all options are compatible), pyomo_unit would automatically match the newly set oli_unit; conversely, setting the pyomo_unit to a new value (string or pint units), would automatically update the corresponding oli_unit.

My issue is that this does not work for all cases because the OLI units are bespoke, e.g., #1388 (comment), and that issue aside, according to the documentation OLI will only take a fixed set of units for each parameter.

Because of this, I would suggest if the user wants different units to go to OLI we make them specify them as OLI unit / pint unit pair.

Right- so, I think I have a solution for this, but that would come as a later PR if we decided to handle that way. Essentially, the same way we have valid_phases that OLI accepts, for each OLI property, we'd define valid_units. That would take more time/effort but can be handled separately. I also am not too crazy about needing to specify a pair. In any case, maybe the first thing to do is think through the use-case for pyomo_unit with a mini working example and see whether we want to keep/change. From that, if it becomes clear that we don't need to have pyomo_unit for whatever reason, then we wouldn't have wasted time on it.

But on this PR- it sounds like you'd opt to strip the changes to FixedKeysDict from here. I would reinject it in a subsequent PR for a common use case that WaterTAP users (myself included) might want to run often, and we can take it from there. How does that sound?

adam-a-a · 2024-05-24T16:37:14Z

@OOAmusat @avdudchenko I think you two are the only ones who could add reviews to this PR at this point. Please take a look if you have a chance. If this is something you don't think you'll have time for this quarter, let me know, and I will merge without reviews (because we really should have the major fix included in this PR as part of 1.0).

…into fix_oli_client

OOAmusat · 2024-05-30T17:24:20Z

watertap/tools/oli_api/client.py

            if invalid_phases:
                raise RuntimeError(
                    "Failed DBS file generation. "
                    + f"Unexpected phase(s): {invalid_phases}"
                )
            dbs_file_inputs["phases"] = phases
        else:
-            dbs_file_inputs["phases"] = ["liquid1", "solid"]
+            dbs_file_inputs["phases"] = ["liquid1", "solid", "vapor"]


@adam-a-a Is there a reason why we do not have "liquid2" here?

For now, because we haven't experimented with it enough and I didn't want to open up more cans of worms until we get the more fundamental bits of code working, I think it is safer to add liquid2 as part of the default in a later PR, which would need to include some thorough testing to see how various OLI functions may or may not be impacted.

adam-a-a · 2024-05-30T21:04:01Z

I have tests passing for OLI. @TimBartholomew @OOAmusat @avdudchenko - just a reminder that I would like to get this merged for the June release since it fixes some issues that needed repair. Hoping to get your reviews in time; otherwise, we may just end up merging without the full 2 reviews.

OOAmusat

LGTM.

Would have liked to test the units switching myself to check for edge cases, but in the absence of OLI access, I guess this is the best we can do.

avdudchenko

This looks good to me - nothing really popped out that would cause issues. LTMG

adam-a-a added 4 commits April 24, 2024 15:39

raise exception when vapor not included and bubblepoint function called

cf08c78

Merge branch 'main' into fix_oli_client

2c3e6bf

commit changes to fixedkeysdict and debugging flash for vapordiffusiv…

d58b4e6

…itymatrix results; reminder to check diff and remove debugging bits and finalized fixedkeysdict changes

handle vapordiffusivitymatrix in flatten_results method; add test for…

a436577

… bubblepoint; test fixedkeysdict funcitonality with resolved units issue

adam-a-a commented May 14, 2024

View reviewed changes

watertap/tools/oli_api/flash.py Outdated Show resolved Hide resolved

adam-a-a added 2 commits May 14, 2024 12:23

Update watertap/tools/oli_api/flash.py

efaa94d

blk

541264b

adam-a-a requested review from veccp, bknueven, avdudchenko and OOAmusat May 14, 2024 19:38

adam-a-a self-assigned this May 14, 2024

adam-a-a added the oli label May 14, 2024

adam-a-a added 2 commits May 14, 2024 16:46

repair tests/pylint check

049ec04

Merge branch 'main' into fix_oli_client

197fe34

bknueven reviewed May 15, 2024

View reviewed changes

adam-a-a added the 1.0 Hard requirement for the 1.0 release label May 16, 2024

adam-a-a added 2 commits May 16, 2024 00:37

add vapor to list of default phases when none are specigied for dbs g…

62be05f

…eneration

Merge branch 'fix_oli_client' of https://github.com/adam-a-a/watertap …

4875110

…into fix_oli_client

ksbeattie added the Priority:High High Priority Issue or PR label May 16, 2024

adam-a-a mentioned this pull request May 28, 2024

Improve OLI API Credential Manager #1300

Closed

adam-a-a added 9 commits May 28, 2024 16:55

Merge branch 'main' into fix_oli_client

a267578

try testing invalid phase

aec8734

Merge branch 'fix_oli_client' of https://github.com/adam-a-a/watertap …

a2c9f71

…into fix_oli_client

black

18fe097

fix test

b27ff49

make invalid phase a list entry

510fb39

fix string expected

2d79077

re.escape()

7f81ff5

blk

b5bcc5c

OOAmusat reviewed May 30, 2024

View reviewed changes

OOAmusat approved these changes May 30, 2024

View reviewed changes

avdudchenko approved these changes Jun 6, 2024

View reviewed changes

Merge branch 'main' into fix_oli_client

bf19550

bknueven enabled auto-merge (squash) June 10, 2024 17:27

bknueven merged commit aefad78 into watertap-org:main Jun 10, 2024
22 of 24 checks passed

This was referenced Jun 28, 2024

Enable concentration factor survey in OLI API #1458

Merged

Feature Requests for OLI API #1328

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bubblepoint function and unit inconsistency in OLI API #1388

Fix bubblepoint function and unit inconsistency in OLI API #1388

adam-a-a commented May 14, 2024 •

edited

Loading

codecov bot commented May 14, 2024 •

edited

Loading

bknueven left a comment

bknueven May 15, 2024

bknueven May 15, 2024

adam-a-a May 16, 2024

adam-a-a commented May 16, 2024

bknueven commented May 16, 2024

adam-a-a commented May 16, 2024

bknueven commented May 16, 2024

adam-a-a commented May 16, 2024

bknueven commented May 16, 2024

adam-a-a commented May 16, 2024 •

edited

Loading

adam-a-a commented May 24, 2024 •

edited

Loading

OOAmusat May 30, 2024 •

edited

Loading

adam-a-a May 30, 2024

adam-a-a commented May 30, 2024

OOAmusat left a comment •

edited

Loading

avdudchenko left a comment

Fix bubblepoint function and unit inconsistency in OLI API #1388

Fix bubblepoint function and unit inconsistency in OLI API #1388

Conversation

adam-a-a commented May 14, 2024 • edited Loading

Fixes/Resolves:

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented May 14, 2024 • edited Loading

Codecov Report

bknueven left a comment

Choose a reason for hiding this comment

bknueven May 15, 2024

Choose a reason for hiding this comment

bknueven May 15, 2024

Choose a reason for hiding this comment

adam-a-a May 16, 2024

Choose a reason for hiding this comment

adam-a-a commented May 16, 2024

bknueven commented May 16, 2024

adam-a-a commented May 16, 2024

bknueven commented May 16, 2024

adam-a-a commented May 16, 2024

bknueven commented May 16, 2024

adam-a-a commented May 16, 2024 • edited Loading

adam-a-a commented May 24, 2024 • edited Loading

OOAmusat May 30, 2024 • edited Loading

Choose a reason for hiding this comment

adam-a-a May 30, 2024

Choose a reason for hiding this comment

adam-a-a commented May 30, 2024

OOAmusat left a comment • edited Loading

Choose a reason for hiding this comment

avdudchenko left a comment

Choose a reason for hiding this comment

adam-a-a commented May 14, 2024 •

edited

Loading

codecov bot commented May 14, 2024 •

edited

Loading

adam-a-a commented May 16, 2024 •

edited

Loading

adam-a-a commented May 24, 2024 •

edited

Loading

OOAmusat May 30, 2024 •

edited

Loading

OOAmusat left a comment •

edited

Loading