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

Enable concentration factor survey in OLI API #1458

Merged
merged 7 commits into from
Jun 29, 2024

Conversation

adam-a-a
Copy link
Contributor

Fixes/Resolves:

Summary/Motivation:

One way to streamline a composition survey in OLI API, where we want to simulate the concentrating of solutes in solution as water is removed from that solution is to conduct a survey on H2O itself (as opposed to applying concentration factors to each solute in composition via a survey).

However, as our oli_api/client and oli_api/flash are configured now, this is not possible. This PR makes some adjustments to set the stage for conducting a concentration factor survey in a future PR by dealing with units as OLI would expect them to be.

Changes proposed in this PR:

  • add "inflows" to FixedKeysDict input_unit_set
  • alter the Flash code so that it propagates units from "inflows" allowing the user to change "molecularConcentration" without unintended consequences
  • subtle cleanup and testing

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.

Comment on lines +331 to +339
conc_unit = self.input_unit_set["inflows"]["oli_unit"]
_logger.info(f"Using {conc_unit} for inflows input")
for k, v in inflows.items():
charge = get_charge(k)
input_list.append(
{
"group": get_charge_group(charge),
"name": get_oli_name(k),
"unit": self.input_unit_set["molecularConcentration"]["oli_unit"],
"unit": conc_unit,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

most important changes of the PR. Simple fix

Comment on lines +576 to +582
if ("unit" in inflows.keys()) and ("values" in inflows.keys()):
input_dict["inflows"] = inflows
# otherwise, assume a solute dictionary with {solute_name: concentration_value}
else:
unit = self.input_unit_set["molecularConcentration"]["oli_unit"]
inflows_oli_names = {get_oli_name(k): v for k, v in inflows.items()}
input_dict["inflows"] = {"unit": unit, "values": inflows_oli_names}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another important change to allow for the user to just supply a solute:concentration dict to the configure_flash_analysis method for setting up isothermal flash calculations.

This is what we do for our configure_water_analysis method, and this configure_flash_analysis seems to assume it'll be getting output from get_apparent_species_from_true; i.e., without this PR, the code assumes that the user is running a water analysis first and then extracting apparent species output as input to this method; either that, or the user needs to manually supply a dict that doesn't match the input dict format that goes into configure_water_analysis.

This standardizes things to make things less confusing for the user.

Comment on lines +817 to +819
output_unit_set_info = {}
for k, v in self.output_unit_set.items():
output_unit_set_info[k] = v["oli_unit"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I turned output_unit_set into a nested FixedKeysDict, which basically means that (1) users can't overwrite keys, which makes sense because these keys correspond to specific OLI output property names, and (2) setting the oli_unit or the pyomo_unit of a given property will sync with both oli_unit and pyomo_unit values. See #1388 for more background if that doesn't make sense.

@adam-a-a adam-a-a added 🍒 Merge to rel Merge commit(s) from this PR to release branch 1.0 Hard requirement for the 1.0 release oli labels Jun 28, 2024
@adam-a-a adam-a-a mentioned this pull request Jun 28, 2024
12 tasks
Copy link
Contributor

@avdudchenko avdudchenko left a comment

Choose a reason for hiding this comment

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

LGMT

Copy link
Contributor

@OOAmusat OOAmusat left a comment

Choose a reason for hiding this comment

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

I think this is fine.

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.16%. Comparing base (e61723a) to head (39ecc97).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
watertap/tools/oli_api/flash.py 60.00% 4 Missing ⚠️
watertap/tools/oli_api/util/fixed_keys_dict.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1458      +/-   ##
==========================================
+ Coverage   93.67%   95.16%   +1.49%     
==========================================
  Files         285      285              
  Lines       30572    30580       +8     
==========================================
+ Hits        28638    29102     +464     
+ Misses       1934     1478     -456     

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

@adam-a-a adam-a-a enabled auto-merge (squash) June 29, 2024 03:20
@adam-a-a adam-a-a merged commit 466995f into watertap-org:main Jun 29, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Hard requirement for the 1.0 release 🍒 Merge to rel Merge commit(s) from this PR to release branch oli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants