fix: resolve holistic review findings for 1.9.3#26
Merged
Conversation
Guard QuantityFrame.scan_parquet against an upstream polars panic: the Parquet statistics reader has no implementation for extension dtypes, so a pushed-down predicate on a unit-typed column aborts a worker thread. Every unit-typed column is re-wrapped through a metadata-only identity that the optimizer will not push a predicate through, and the upstream bug itself is pinned by a test. Reject non-numeric numpy magnitudes. _cast_array_float now allow-lists the float, signed and unsigned integer dtype kinds instead of relying on an unsafe cast, so a bool mask, a complex array (previously silently losing its imaginary part) and datetime64/timedelta64 (previously silently becoming tick counts) are refused with the same messages the pl.Series path already used. Stop testing dimensionality with an O(n) magnitude pass. pint's quantity-level `dimensionless` converts the whole magnitude to root units; the unit-level check is magnitude-independent and answers identically for every registered unit. Applied in __mul__, __truediv__, __floordiv__ and check_compatibility, with the isinstance test reordered first so `q * 2.0` skips it entirely. Validate CoolProp expression requests eagerly: cp.fluid now checks the output property and the fluid name where the expression is built rather than at collect() time inside a plugin node. The name check moved out of encomp.fluids into encomp.coolprop as public validate_fluid_config (cached) / validate_fluid_name, so both entry points share one implementation. Other fixes: ndim reports 1 for pl.Series and pl.Expr magnitudes; round() works for every magnitude container; units_of degrades per column and warns when a foreign producer wrote unparseable unit metadata; QuantityFrame has a collection-free repr; typed-read and with_units errors name the offending column; the division-by-zero divergence between containers is pinned as deliberate; a dangling ". " is gone. Rename fluids.BackendKind/BACKEND_KIND to NativeInterface/NATIVE_INTERFACE: it selects the native interface (AbstractState vs HAPropsSI), so "backend" now refers only to the CoolProp equation-of-state backend everywhere. Docs: correct two stale claims about bare `import encomp` (a bad .env value and the application-registry takeover both need a submodule import), the wheel-size figure, and note that omitting an INCOMP concentration means 0 % solute; add a scalar-vs-vector performance note, the scan-and-filter limitation, and a getting-started notebook pointer. Upgrade the locked dependencies (polars 1.43.0, ruff 0.16.0, ty 0.0.63) and bump the version to 1.9.3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Missing values: polars distinguishes null (absent) from NaN (an ordinary float
value), numpy has only NaN for both, and the two conventions met without a stated
rule. The rule is now explicit and implemented: encomp never produces a NaN in a
polars magnitude (a CoolProp state that cannot be fixed is null, as before), a NaN
the caller puts in a Series is kept verbatim because it is data rather than a
missing marker, and astype -- the boundary between the two worlds -- translates the
sentinel. astype("pl.Series") previously carried a numpy NaN straight into polars,
where it poisons mean() and is invisible to drop_nulls(); it now becomes null, and
the reverse direction already rendered null as NaN.
Temperature vs TemperatureDifference: a Quantity[Temperature] could carry a delta
unit, so its unit and its dimensionality stated different physical meanings
(Q[Temperature, float](5.0, "delta_degC"), and asdim(Temperature) on a difference,
both produced 5 Δ°C typed as an absolute temperature). That pair is now re-resolved
from the unit, exactly as every other mismatched subclass hint is
(Q[Mass](1, "m") is a Length) -- it cannot raise, because pint itself builds T - T
as self.__class__(magnitude, delta_unit) on the absolute class. asdim() gained the
mirror of the existing rewrite, so reinterpreting a difference as absolute moves
delta_degC back to degC; only the genuinely ambiguous multiplicative units (K,
degR, ...) keep their spelling under both readings.
A difference also converts to K by scale alone, which let it fix a CoolProp state:
300 ΔK silently flashed as 300 K, and 5 Δ°C returned NaN instead of an error. No
CoolProp state input is a difference, so to_numeric_correct_unit (the single place
state-input dimensionality is validated) refuses the whole class, as does
ideal_gas_density -- its sibling _resolve_gas_condition already did.
Both rules are documented in the usage guide and the Quantity docstring, and pinned
by tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…te the null sentinel for pl.Expr
numpy dispatch never reaches __sub__/__add__, and pint's ufunc path rebuilds the result
from the unit alone -- so np.diff / np.ediff1d / np.gradient / np.ptp / np.std of a
kelvin array came back typed as an absolute Temperature, and such a difference could
then fix a CoolProp state as an absolute temperature: exactly the hole the previous
commit closed for the operators. np.subtract and np.add now delegate to `-` and `+`, so
the rules have one implementation (np.add on two degC quantities consequently raises
OffsetUnitCalculusError like the operator instead of answering 45 °C), and the
difference-producing array functions re-type their result. degC was already correct
because pint emits delta_degC there; K is what leaked.
astype("pl.Expr") carried a NaN into the polars world while astype("pl.Series")
translated it to null, although the docstring names astype as *the* translating
boundary. Both branches now translate.
Two consistency fixes around the dimensionality classes: the T ± ΔT arithmetic tested
its operands with identity rather than issubclass, so a user-defined subclass took part
in every other T/ΔT rule but not in the arithmetic that defines the concept; and
_distinct is no longer inherited as a claim -- a `class MyTemperature(Temperature)`
used to take over unit resolution for [temperature] process-wide, silently retyping
every temperature quantity in the process. An inherited `_distinct = False` is still
honoured, since IndistinctDimensionality exists to be subclassed.
convert_volume_mass' docstring promised that "a Polars result carries null (never NaN)"
while a NaN in inp propagates as IEEE float data through the arithmetic. The guard is
about the density argument, where NaN means *missing*; the docstring now says that
instead of overreaching, and both behaviors are pinned.
The rules behind all of this are lifted into AGENTS.md (library invariants) and the
usage guide, which now also covers the numpy-dispatch typing, the one-way collapse of a
data-NaN through numpy and CoolProp, and the K + K asymmetry. Also fills the test gaps
the review listed: numpy dispatch, null/NaN through the parquet round-trip, the
polars->numpy->polars direction, an offset unit declared as a difference column, and
temperature cases in test_comparisons.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Implements every item from the 1.9.2 holistic review (findings 1-12) plus six more found in a follow-up sweep of the whole library (13-18). Every finding was reproduced at runtime before being fixed.
Correctness
scan_parquet+ filter (finding 1). Polars' Parquet statistics reader has no implementation for extension dtypes, so a predicate pushed into a scan on a unit-typed column aborts a worker thread with a RustPanicException(1.42 and 1.43; parquet-only,statistics=Falsedoes not help).QuantityFrame.scan_parquetnow re-wraps every unit-typed column through a metadata-only identity the optimizer will not push a predicate through, and a test pins the upstream bug so a polars fix fails loudly._cast_array_floatcast withcasting="unsafe", so a bool mask became1.0/0.0, a complex array silently lost its imaginary part (numpyComplexWarningonly), anddatetime64/timedelta64became raw tick counts — all of which thepl.Seriespath already refused. The dtype kinds are now allow-listed.Performance
dimensionlessprobe on the hottest operators (findings 3, 15). pint's quantity-level property converts the whole magnitude to root units; measured at 1e6 elements it is 145 µs (ndarray) / 485 µs (Series) versus 1.8 µs for the unit-level check, and it ran even forq * 2.0where the branch can never be taken. Replaced in__mul__,__truediv__,__floordiv__andcheck_compatibility(which is reached by+,-,==,!=and ordering against a plain number). The two checks were verified equivalent over all 1061 units in the registry plus the offset/delta/custom-dimension spellings.Ergonomics
cp.fluidvalidates the output property and the fluid name eagerly instead of failing atcollect()inside a plugin node (finding 7). The name check moved out ofencomp.fluidsintoencomp.coolpropas publicvalidate_fluid_config(cached) /validate_fluid_name, so both entry points share one implementation.ndimreturns 1 forpl.Series/pl.Exprmagnitudes, which previously answered 0 whilelen()returned the length (finding 14).round()works for every magnitude container (finding 18).units_ofdegrades per column and warns when a foreign producer wrote unparseable unit metadata, instead of making the whole frame uninspectable (finding 5).QuantityFramehas a collection-freerepr(finding 8); typed-read andwith_unitserrors name the offending column (findings 9, 16); a dangling". "is gone (finding 11).fluids.BackendKind/BACKEND_KINDrenamed toNativeInterface/NATIVE_INTERFACE: it selects the native interface (AbstractStatevsHAPropsSI), so "backend" now means only the CoolProp equation-of-state backend anywhere in the codebase.Docs
Two stale claims about bare
import encompcorrected — a bad.envvalue and the application-registry takeover both require a submodule import now that the top-level package is__version__only (findings 6, 17); wheel-size figure refreshed (finding 10);Fluiddocstring says that omitting an INCOMP concentration means 0 % solute (finding 12); plus a scalar-vs-vector performance note, the scan-and-filter limitation, and a getting-started notebook pointer.Dependencies
uv lock --upgrade(polars 1.43.0, ruff 0.16.0, ty 0.0.63, and 22 others). ty 0.0.63 made threety: ignorecomments unnecessary (removed) and added one false positive on theqty.msetter (suppressed); ruff 0.16.0 reformats one markdown code block.Gates
pytest 1158 passed (the CoolProp oracle group is installed locally, so nothing skipped) · pyright 0 errors · pyrefly 0 errors · ty all checks passed · ruff check + format clean ·
sphinx-build -Wsucceeded.🤖 Generated with Claude Code