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

Automatically load MW and charge in MCAS #1246

Merged
merged 14 commits into from
Dec 14, 2023

Conversation

adam-a-a
Copy link
Contributor

@adam-a-a adam-a-a commented Dec 8, 2023

Fixes/Resolves:

Summary/Motivation:

This PR allows the user to now skip the step of entering MW and charge data for solutes---assuming they follow proper naming convention. If a name doesn't line up with what is expected, the user could manually enter data for those solutes but skip for more typical solute names (e,g,, "Na_+" and similar ions can be handled without a problem, but "target_ion" wouldn't turn up search results).

Changes proposed in this PR:

  • load mw data by default if missing
  • load charge data by default if missing
  • bolster some of the watertap_to_oli helper functions with exceptions
  • add testing for helper functions
  • add get_molar_mass_quantity function to assign pint units to MW

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.

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (fa126c5) 94.28% compared to head (60f2480) 94.27%.

Files Patch % Lines
...s/oli_api/util/watertap_to_oli_helper_functions.py 89.47% 2 Missing ⚠️
...rtap/property_models/multicomp_aq_sol_prop_pack.py 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1246      +/-   ##
==========================================
- Coverage   94.28%   94.27%   -0.01%     
==========================================
  Files         357      357              
  Lines       35894    35918      +24     
==========================================
+ Hits        33841    33863      +22     
- Misses       2053     2055       +2     

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

):
m[0].fs.properties = MCASParameterBlock(
solute_list=["Na_+", "Cl_-", "N"],
mw_data={"H2O": 0.018, "Na_+": 0.023, "Cl_-": 0.0355, "N": 0.01},
Copy link
Contributor

Choose a reason for hiding this comment

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

minor observation - mw for N is 0.014

Copy link
Contributor Author

@adam-a-a adam-a-a Dec 14, 2023

Choose a reason for hiding this comment

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

True - although I wasn't too careful when doing this since the tests are really intended to test functionality rather than true values (in these particular instances).

Copy link
Contributor

@veccp veccp left a comment

Choose a reason for hiding this comment

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

looks good to me

@adam-a-a adam-a-a self-assigned this Dec 14, 2023
@adam-a-a adam-a-a added Priority:Normal Normal Priority Issue or PR property_model labels Dec 14, 2023
Copy link
Contributor

@kurbansitterley kurbansitterley left a comment

Choose a reason for hiding this comment

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

LGTM

@ksbeattie ksbeattie merged commit 17d9679 into watertap-org:main Dec 14, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR property_model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants