OLI API Flash update #1296
OLI API Flash update #1296
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1296 +/- ##
==========================================
+ Coverage 94.25% 94.70% +0.45%
==========================================
Files 370 370
Lines 37950 38229 +279
==========================================
+ Hits 35768 36204 +436
+ Misses 2182 2025 -157 ☔ View full report in Codecov by Sentry. |
| " # if a DBS file has been retained from a previous session,\n", | ||
| " # its flash history and chemistry information can be summarized.\n", | ||
| " file_summary = oliapi.get_dbs_file_summary(dbs_file_id)\n", | ||
| "\n", | ||
| " # save chemistry information\n", | ||
| " chemistry_info = file_summary[\"chemistry_info\"]\n", | ||
| " write_output(chemistry_info[\"result\"], \"chemistry_info\")\n", |
There was a problem hiding this comment.
This bit on getting the summary/flash history should be shown later, after walking through the main essentials.
|
@lbianchi-lbl Fix (expected) failures in notebooks tests. |
|
The OLI API CI notebooks job is failing seemingly because of `interactive_mode=True); maybe some change has been inadvertently reverted in the notebook file? EDIT It seems so, judging from b982cbd
|
|
@adam-a-a For reference, this is how the OLI API CI checks look like:
|
This is done in bfcb306, which adds functionality to skip/xfail OLI notebooks if the credentials are not found in the environment:
|
|
OK, I think I managed to get the Codecov upload to work:
|
|
The OLI API CI jobs now also create a "pending" status check when the workflow is started: |
ksbeattie
added
Priority:High
| "optionalProperties": dict(self.optional_properties), | ||
| "unitSetInfo": dict(self.output_unit_set), | ||
| } | ||
| if included_solids and excluded_solids: |
There was a problem hiding this comment.
Is there a reason why both cannot be specified at the same time?
There was a problem hiding this comment.
As per OLI:
params.excludedSolids and params.includedSolids cannot be specified at the same time.
| param | ||
| ] | ||
| elif param in modified_clone["params"]["inflows"]["values"]: | ||
| d = d["waterAnalysisInputs"] |
There was a problem hiding this comment.
why did you remove the temperature/pressure catch here? If it is removed, then the relative/absolute value boolean for the survey definitely needs to be multi-dimensional.
If a user wants to sweep across temperature (more likely to be absolute) and concentration (more likely to be relative), how would this work? If T/P are handled explicitly as previously, we could maybe avoid that issue. As it is now, we definitely need to deal with it.
There was a problem hiding this comment.
Makes sense - I'll make note to reimplement the catch for T/P and also pH
Co-authored-by: Adam Atia <[email protected]>
There was a problem hiding this comment.
LGTM - we will resolve any outstanding issues in subsequent PRs.
I want to reiterate that before we continue adding any new or altered functionality (or API changes in general), we should prioritize adding tests to cover the wide range of functionality introduced in this and previous PRs.
Fixes/Resolves:
This addresses some issues of #1279, dealing mainly with the Flash and OLIApi classes and adding test coverage for new functionality.
E.g.: move more general functions out of Flash class, rework Client and Flash frontend, improve compatibility with async #1303...
Summary/Motivation:
To allow for other flash calculations and frameworks to be used with OLI Cloud, some streamlining of functions was necessary.
Changes proposed in this PR:
Aqueous (H+ ion)thermo framework (required for corrosion analyzer)XSCprivate databank (required for surface complexation calculations, silica scaling)incorporating_oli_calculationsLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: