Skip to content

perf(get_amount): add float fast path for common units - #454

Open
rkingsbury wants to merge 1 commit into
mainfrom
rk/get-amount-fast-path
Open

perf(get_amount): add float fast path for common units#454
rkingsbury wants to merge 1 commit into
mainfrom
rk/get-amount-fast-path

Conversation

@rkingsbury

Copy link
Copy Markdown
Member

Summary

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 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 single Quantity construction, and returns None for anything else so get_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 a Quantity per solute.
  • dielectric_constant hoists the mole-fraction denominator out of its loop — O(N) instead of O(N²), since get_amount(.., "fraction") re-summed all components on every call.

alkalinity, hardness, and ionic_strength speed up with no code change — they call get_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/L as (n/V)*mw, not n*mw/V (they differ by 1 ULP for e.g. Mg²⁺). M is deliberately left on the pint path (its conversion differs from mol/L by 1 ULP).

  • 125 (solute, unit) combos: 0 mismatches in both magnitude and units vs. the forced pint path.
  • No unit tests changed; public API unchangedget_amount keeps its signature, return type, units, and values. The exact == alias assertions in test_get_amount (ppm == mg/L, eq/L == M, ...) still hold.
  • Full suite green: 1986 tests pass across 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 raised UndefinedUnitError on the reference path (the except DimensionalityError there does not catch it); it now returns 0 (dimensionless), the sensible mole fraction of an absent solute. No test exercised this.

Benchmark — 33-component solution (bit-identical output)

property baseline modified speedup
charge_balance 9.46 ms 0.34 ms 28x
mass 12.12 ms 0.57 ms 21x
total_dissolved_solids 14.56 ms 0.94 ms 15x
dielectric_constant 5.76 ms 0.38 ms 15x (O(N²)→O(N))
conductivity 11.80 ms 1.84 ms 6.4x
alkalinity 4.83 ms 0.75 ms 6.4x
hardness 3.36 ms 0.66 ms 5.1x
ionic_strength 0.59 ms 0.28 ms 2.1x

Construction of the same solution also dropped ~36% (93 → 60 ms).

🤖 Generated with Claude Code

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

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.30159% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.31%. Comparing base (4f14b17) to head (a59935e).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/pyEQL/solution.py 87.30% 6 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant