perf(get_amount): add float fast path for common units - #454
Open
rkingsbury wants to merge 1 commit into
Open
Conversation
get_amount() built a pint Quantity and ran context-based .to() conversions (plus qty.check dimensionality probes) on every call. In the O(N) property sweeps that iterate over all components (mass, charge_balance, conductivity, total_dissolved_solids, alkalinity, hardness, ...) this per-solute pint overhead dominated. Add _amount_fast(), which computes the common units (mol, mol/L, mol/kg/m, g, kg, g/L, mg/L, fraction, eq, eq/L) with plain float arithmetic and a single Quantity construction, falling back to the unchanged pint path for anything else (M, count, %, SI-prefixed variants, ...). The arithmetic was verified bit-for-bit against pint across solutes -- including that pint evaluates g/L and mg/L as (n/V)*mw, not n*mw/V (they differ by 1 ULP for some solutes). "M" is intentionally left on the pint path (its conversion differs from mol/L by 1 ULP). Also: - _get_amount_mag() returns the bare float; hot loops (mass, charge_balance, conductivity, total_dissolved_solids) use it to skip per-solute Quantity construction. - dielectric_constant: hoist the mole-fraction denominator out of the loop, making it O(N) instead of O(N**2) (get_amount(..,"fraction") re-summed all components on every call). Results are unchanged: public get_amount keeps its signature, return type, units, and values (verified over 125 solute/unit combos; the full test suite passes with no test changes). Incidentally, get_amount(<absent>, "fraction") now returns 0 instead of raising UndefinedUnitError from the reference path. Per-property speedups on a 33-component solution (bit-identical output): charge_balance 28x, mass 21x, total_dissolved_solids 15x, dielectric 15x, conductivity 6.4x, alkalinity 6.4x, hardness 5.1x, ionic_strength 2.1x. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
==========================================
- Coverage 87.81% 87.31% -0.51%
==========================================
Files 14 14
Lines 1945 2002 +57
Branches 338 354 +16
==========================================
+ Hits 1708 1748 +40
- Misses 188 200 +12
- Partials 49 54 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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
get_amount()built a pintQuantityand ran context-based.to()conversions (plusqty.check()dimensionality probes) on every call. In the O(N) property sweeps that iterate over all components —mass,charge_balance,conductivity,total_dissolved_solids,alkalinity,hardness, ... — this per-solute pint overhead dominated the cost.This adds a float fast path:
_amount_fast()computes the common units (mol,mol/L,mol/kg/m,g,kg,g/L,mg/L,fraction,eq,eq/L) with plain float arithmetic and a singleQuantityconstruction, and returnsNonefor anything else soget_amount()falls through to the unchanged reference path (M,count,%, SI-prefixed variants, ...)._get_amount_mag()returns the bare float; the hot loops use it to skip building aQuantityper solute.dielectric_constanthoists the mole-fraction denominator out of its loop — O(N) instead of O(N²), sinceget_amount(.., "fraction")re-summed all components on every call.alkalinity,hardness, andionic_strengthspeed up with no code change — they callget_amount/solvent_mass, which now hit the fast path.Correctness
The fast-path arithmetic is bit-identical to pint, verified by probing pint's exact operation order — including the non-obvious detail that pint evaluates
g/L/mg/Las(n/V)*mw, notn*mw/V(they differ by 1 ULP for e.g. Mg²⁺).Mis deliberately left on the pint path (its conversion differs frommol/Lby 1 ULP).get_amountkeeps its signature, return type, units, and values. The exact==alias assertions intest_get_amount(ppm == mg/L,eq/L == M, ...) still hold.test_solution,test_utils, the phreeqc engines, and the density/dielectric/debye/osmotic/activity/volume/salt-matching files (12 skipped / 2 xfail are pre-existing and unrelated).Incidental fix:
get_amount(<absent solute>, "fraction")previously raisedUndefinedUnitErroron the reference path (theexcept DimensionalityErrorthere does not catch it); it now returns0(dimensionless), the sensible mole fraction of an absent solute. No test exercised this.Benchmark — 33-component solution (bit-identical output)
Construction of the same solution also dropped ~36% (93 → 60 ms).
🤖 Generated with Claude Code