Add benchmarking suite#252
Closed
ugognw wants to merge 57 commits into
Closed
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
- Coverage 81.79% 80.01% -1.79%
==========================================
Files 9 10 +1
Lines 1494 1621 +127
Branches 257 282 +25
==========================================
+ Hits 1222 1297 +75
- Misses 226 277 +51
- Partials 46 47 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
Wow, thank you for the effort on this @ugognw! It looks well thought out. I'm traveling this week and part of next week but will dedicate some time to reviewing in detail as soon as I can. |
ugognw
force-pushed
the
benchmarking-suite
branch
from
c1da2ed to
5735dea
Compare
Contributor
Author
|
Thanks! I'd like to think so! Sounds good. I look forward to hearing your feedback! |
Signed-off-by: Ugochukwu Nwosu <ugn@sfu.ca>
Replace ReferenceData with _BenchmarkEntry. This named tuple now separates solution and solute properties which enables the separate retrieval of each type of property. An analogous _get_solution_property method has been implemented. _get_reference is renamed to _get_dataset as the data structure returned by the method defines a general solution/solute property data set (reference data + Solution object which can be used for benchmarking). benchmark_engine functionality moved to report_results for simplicity. If more fine-tuned control over data set partitioning or result reporting is desired, this can be implemented quite readily by undoing this change. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Solution creation is now handed when reference files are read. Benchmarking is parametrized by engine. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Although EOS subclasses define get_activity_coefficicient to return the activity coefficients of individual solutes, one can only measure the mean activity coefficient experimentally. As such, to benchmark against experimental data, we must use the definition of the mean activity coefficient in terms of the activity coefficients of the individual ions. Alternatively, this function could be implemented as a property (Solution.mean_activity) or a method (Solution.get_mean_activity). Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Publicize functions/classes for processing input/output data models Return values from main function (renamed to benchmark_engine for clarity) and pass engine and source as arguments. Store results instead of printing to console. Parse Solution dictionary into Solution object when reading dataset from file. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Passing a string corresponding to an internal source name retrieves the Path to the data file (distributed with package). All files are loaded in the same way. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
...to list[tuple[str, Quantity]] Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
The new RMSE is more robust in the case that property values are Python primitives. Property getters are now called to retrieve properties. This fixes a bug where the getter was returned instead of the value. Variables are renamed (from property) to avoid shadowing a Python built-in. Enumerating solution data (in get_dataset) yields the right number of items to unpack. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Improve docstrings Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Salt takes no extra kwargs. reduce takes no kwargs. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
FormulaDict was initially used in order to automatically standardize how solutes appear in solution compositions and solute property lists. However, in addition to standardizing ion formulas, FormulaDict also sorts its internal copy of the dictionary by its values. This led to an error in the previous implementation because the values are dictionaries, and dictionaries don't implement the required methods for comparison with '<' and '>'. Instead, we manually call standardize_formula in the public functions where malformatted formulas are likely. This is when loading datasets and when creating a BenchmarkEntry from a Solution. Bugs: initialize property data as dictionaries in create_entry; fix indices and data type in _create_engine_dataset; initialize data pair lists in calculate_stats. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
The concentration for Solution objects that are created in _create_engine_dataset is determined from the SolutionKey. Without units, the amounts are not interpreted as concentrations and the Solution does not have the correct composition. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
The three load_dataset unit tests are just outlines and are expected to fail. The explicit test cases for test_should_benchmark_all_engines are intended to eventually overlap with those in test_conductivity and eventually replace them. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
The number of moles of solvent is dependent on the engine, but in the capacity that SolutionKey will be used, two Solutions with the same concentrations of their solutes with differing engines (and thus differing moles of solvent) should be considered identical if they are at the same state (temperature and pressure). Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Updated module docstring variable for consistency. Require both _create_engine_dataset arguments. For now, only require that the metric argument to calculate_stats can handle Quantity arguments, so that we can ensure that units are handled correctly. Note that the property values in BenchmarkEntry.solute_data and BenchmarkEntry.solution_data are expected to be Quantity objects. Also, the additional conditional statements in calculate_stats serve to handle the edge cases of when the return values of _get_solute_property and _get_solution_property are None and when there does not exist data in `calculated` corresponding to `reference`. Because this may be of intereste to the user, the event is logged at the INFO level. Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
Signed-off-by: Ugochukwu Nwosu <ugognw@gmail.com>
ugognw
force-pushed
the
benchmarking-suite
branch
from
fb3df4c to
82e06df
Compare
Member
Update: I will do my best to look at this next week. I have some proposals due at the end of this week that will keep me busy until then. Thanks for your patience! |
YitongPan1
approved these changes
Nov 5, 2025
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Major changes:
pyEQL.benchmarkTodos
validate data
scrutinize dataset format
finalize benchmarking statistics format
remove redundant tests
add more granular unit tests
optimize number of test cases/time
fix PhreeqcEOS benchmarking
add docs under Electrolyte Modeling Engines
remove logic related to
_get_activity_coefficient_pitzer?convert % concentration in datasets to mol/L in load_dataset
Update changelog
Google format doc strings added.
Code linted with
ruff. (For guidance in fixing rule violates, see rule list)Type annotations included. Check with
mypy.Tests added for new features/fixes.
I have run the tests locally and they passed.
Happy to hear any feedback on the current direction!