You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Today data_units is policed by conflict like any other attribute: "raise" refuses patches whose units differ, "drop" and "keep_first" allow them and assembly converts the members to one spelling. The design note (.scratch/kind-empty-rule.md) asks for something different and better:
Units must be commensurable and convert.
That is, units are not a label the conflict policy arbitrates. They scale the data, so the rule is a property of placing patches side by side: metres and kilometres are one quantity spelled two ways and should always concatenate with conversion; metres and seconds are two quantities and should never concatenate, whatever conflict says.
The chunk-merge path (PatchAssembler) streams members into a preallocated buffer. Converting there means rescaling each member as it lands, which is a real change to the streaming merge rather than a metadata fix.
Making units unconditional without the second half produces a worse inconsistency than the current one: Spool.concatenate would convert while Spool.chunk would raise or splice unconverted, for the same two patches.
What it would take
Partition on unit dimensionality rather than exact spelling, so an output never mixes quantities. _normalize_chunk_units already does this for coordinate units; data_units needs the equivalent.
Drop data_units from the conflict-policed columns entirely, and from combine_patch_attrs' fold, since it would no longer be a label.
Incommensurable units then raise from one place with one message, rather than as CoordMergeError at plan time or AttributeMergeError/UnitError at load time depending on the path.
Related
Say on the pull request which lines are not covered #984 fixed the immediate hazard: a unitless first member under keep_first used to suppress conversion, splicing metre- and kilometre-scaled samples into one unlabelled array.
CodeRabbit raised the assembly half of this on Say on the pull request which lines are not covered #984 (concatenate_planned folding data_units through combine_patch_attrs before it can reconcile them); it is correct under the rule above and deliberately out of scope under the current one.
Today
data_unitsis policed byconflictlike any other attribute:"raise"refuses patches whose units differ,"drop"and"keep_first"allow them and assembly converts the members to one spelling. The design note (.scratch/kind-empty-rule.md) asks for something different and better:That is, units are not a label the
conflictpolicy arbitrates. They scale the data, so the rule is a property of placing patches side by side: metres and kilometres are one quantity spelled two ways and should always concatenate with conversion; metres and seconds are two quantities and should never concatenate, whateverconflictsays.Why it isn't done in #984
Two assembly paths would have to converge, and only one of them can convert cheaply today.
concatenate_plannedloads its members whole, so converting is aPatch.convert_unitscall per member. Say on the pull request which lines are not covered #984 does this already for the loosened policies.PatchAssembler) streams members into a preallocated buffer. Converting there means rescaling each member as it lands, which is a real change to the streaming merge rather than a metadata fix.Making units unconditional without the second half produces a worse inconsistency than the current one:
Spool.concatenatewould convert whileSpool.chunkwould raise or splice unconverted, for the same two patches.What it would take
_normalize_chunk_unitsalready does this for coordinate units;data_unitsneeds the equivalent._dtypeaccordingly (integer data floats when converted — Say on the pull request which lines are not covered #984 already handles this for the concat path).data_unitsfrom theconflict-policed columns entirely, and fromcombine_patch_attrs' fold, since it would no longer be a label.CoordMergeErrorat plan time orAttributeMergeError/UnitErrorat load time depending on the path.Related
keep_firstused to suppress conversion, splicing metre- and kilometre-scaled samples into one unlabelled array.concatenate_plannedfoldingdata_unitsthroughcombine_patch_attrsbefore it can reconcile them); it is correct under the rule above and deliberately out of scope under the current one.