Chunk spools by data size and by unit-bearing lengths - #833
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds coordinate-unit and data-size support to chunking and overlap resolution. It records resolved size diagnostics, handles mixed dtypes through upcasting, and persists patch dtypes in schema version 6 indexes and private spool columns. ChangesChunking and dtype propagation
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_io/test_index/test_index_contract.py (1)
213-217: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the
dtypereservation through ingestion.This test checks only the constant. The test can pass while ingestion still accepts an attr named
dtypeand hides or shadows the structural value. Add an end-to-end index test that verifies the intended rejection or omission behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_io/test_index/test_index_contract.py` around lines 213 - 217, Extend test_dtype_attr_is_reserved to construct index input containing an attribute named dtype and ingest it through the public index creation path. Assert the documented rejection or omission behavior, including that the structural dtype value is not shadowed, rather than checking RESERVED_ATTR_COLUMNS directly; reuse existing ingestion fixtures and helpers in the surrounding index contract tests.tests/test_core/test_patch_chunk.py (1)
1055-1060: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe clamped-size test builds a very large number of outputs.
random_spoolholds three patches of roughly 300 x 2000 float64 samples. One time sample costs about 2400 bytes, so a 1 kB request clamps to one sample per output and the plan produces about 6000 output rows. The derived catalog then ingests one record per row beforeout[0]resolves. This makes a single assertion pay for thousands of index writes.Use a small purpose-built patch for this case.
_makealready exists in this class.♻️ Proposed change to shrink the clamped-size case
- def test_slab_larger_than_target_warns(self, random_spool): + def test_slab_larger_than_target_warns(self): """One sample is the floor; the request cannot be honored below one sample.""" + start = np.datetime64("2020-01-01T00:00:00") + # 50 distance samples of float64 = 400 bytes per time sample + spool = dc.spool([self._make("float64", start, distance=50, samples=20)]) with pytest.warns(UserWarning, match="larger than the requested size"): - out = random_spool.chunk(time=1 * dc.units.kB) + out = spool.chunk(time=100 * dc.units.byte) patch = out[0] assert patch.shape[patch.get_axis("time")] == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_core/test_patch_chunk.py` around lines 1055 - 1060, Update test_slab_larger_than_target_warns to use a small purpose-built patch created via the existing _make helper instead of random_spool, while preserving the 1 kB chunk request, warning assertion, and one-time-sample shape assertion.dascore/utils/chunk_plan.py (1)
818-831: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDiagnostics record partitions that produce no outputs.
The loop appends a diagnostics entry at Line 784 before
get_intervalsruns. A partition that is too short raisesChunkErrorat Line 800 and is skipped, but its entry stays insize_diagnostics. The recordedfirst_output_idthen points at anoutput_idthat no row uses, and aclampedentry for a skipped partition still triggers the warning at Line 831.Append the diagnostics after the interval computation succeeds.
♻️ Proposed change to defer the diagnostics append
if per_partition and not merge_mode: ... value_c, overlap_c, diag = _resolve_partition_length( value, overlap, sub, name, size_step, df[min_name].dtype ) - if diag is not None: - size_diagnostics.append({"first_output_id": next_id, **diag}) + else: + diag = None if merge_mode: start_stop = np.atleast_2d(np.asarray([g_start, g_stop])) else: try: start_stop = get_intervals(...) except ChunkError: # partition too short; skip (D8) continue + if diag is not None: + size_diagnostics.append({"first_output_id": next_id, **diag})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dascore/utils/chunk_plan.py` around lines 818 - 831, Move the size-diagnostics append in the chunk-planning loop to after get_intervals completes successfully, while preserving the existing diagnostic contents. Ensure partitions that raise ChunkError and produce no outputs are excluded from size_diagnostics, so their first_output_id and clamped values do not affect the recorded partitions or warning in the size_diagnostics handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_units.py`:
- Around line 553-556: Update test_undefined_byte_alias_raises so the attempted
dc.units.KB lookup is assigned to a local variable within the pytest.raises
block, preserving the UndefinedUnitError assertion while avoiding Ruff B018.
---
Nitpick comments:
In `@dascore/utils/chunk_plan.py`:
- Around line 818-831: Move the size-diagnostics append in the chunk-planning
loop to after get_intervals completes successfully, while preserving the
existing diagnostic contents. Ensure partitions that raise ChunkError and
produce no outputs are excluded from size_diagnostics, so their first_output_id
and clamped values do not affect the recorded partitions or warning in the
size_diagnostics handling.
In `@tests/test_core/test_patch_chunk.py`:
- Around line 1055-1060: Update test_slab_larger_than_target_warns to use a
small purpose-built patch created via the existing _make helper instead of
random_spool, while preserving the 1 kB chunk request, warning assertion, and
one-time-sample shape assertion.
In `@tests/test_io/test_index/test_index_contract.py`:
- Around line 213-217: Extend test_dtype_attr_is_reserved to construct index
input containing an attribute named dtype and ingest it through the public index
creation path. Assert the documented rejection or omission behavior, including
that the structural dtype value is not shadowed, rather than checking
RESERVED_ATTR_COLUMNS directly; reuse existing ingestion fixtures and helpers in
the surrounding index contract tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9a38c5e-0fe0-4367-b90d-4b577d609ca8
📒 Files selected for processing (17)
dascore/core/spool.pydascore/io/index/backend.pydascore/io/index/catalog.pydascore/io/index/indexer.pydascore/io/index/ingest.pydascore/io/index/planned.pydascore/io/index/schema.pydascore/units.pydascore/utils/chunk_plan.pydocs/changelog.qmddocs/notes/spool_chunking.qmddocs/recipes/low_freq_proc.qmddocs/tutorial/spool.qmdtests/test_core/test_patch_chunk.pytests/test_io/test_index/test_index_contract.pytests/test_units.pytests/test_utils/test_chunk.py
| def test_undefined_byte_alias_raises(self): | ||
| """`KB` is not a pint unit; the kilobyte spelling is `kB`.""" | ||
| with pytest.raises(pint.UndefinedUnitError): | ||
| dc.units.KB |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff flags the bare attribute access as a useless expression (B018).
Line 556 evaluates dc.units.KB for its side effect. Ruff cannot infer that the attribute access is the subject under test, so B018 fails the lint job. Bind the result or suppress the rule explicitly.
🧹 Proposed fix for B018
def test_undefined_byte_alias_raises(self):
"""`KB` is not a pint unit; the kilobyte spelling is `kB`."""
with pytest.raises(pint.UndefinedUnitError):
- dc.units.KB
+ _ = dc.units.KB📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_undefined_byte_alias_raises(self): | |
| """`KB` is not a pint unit; the kilobyte spelling is `kB`.""" | |
| with pytest.raises(pint.UndefinedUnitError): | |
| dc.units.KB | |
| def test_undefined_byte_alias_raises(self): | |
| """`KB` is not a pint unit; the kilobyte spelling is `kB`.""" | |
| with pytest.raises(pint.UndefinedUnitError): | |
| _ = dc.units.KB |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 556-556: Found useless expression. Either assign it to a variable or remove it.
(B018)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_units.py` around lines 553 - 556, Update
test_undefined_byte_alias_raises so the attempted dc.units.KB lookup is assigned
to a local variable within the pytest.raises block, preserving the
UndefinedUnitError assertion while avoiding Ruff B018.
Source: Linters/SAST tools
Update after adversarial reviewThree internal reviews (correctness / index plumbing / test quality) found four real defects in this PR, now fixed in The size bound could be exceeded by up to 1.84×. An output holds the sum of its members' sample counts, which equals Correction to this PR's own description: I claimed the bound depends on sizing against the partition's smallest step, and that the median would overshoot. That was true of the pre-packing code but is now wrong — packing is measured in units of whichever step is used, so the two cancel exactly and the length works out to Element dtype is now resolved per output, not per partition. Carrying the partition-wide Non-finite and array-valued quantities are now rejected. NaN could reach the derived catalog as the string Test gaps closedMutation testing found Cross-PR noteThis PR's |
Spool.chunk now accepts a pint quantity as the chunk length. A quantity
in the coordinate's own units works (time=10*s, distance=100*ft), which
previously raised NotImplementedError, and a quantity of information
(time=25*megabytes) chunks so each output patch's data array is at most
the requested size. Overlap accepts both forms.
A size resolves per partition, since the conversion needs that
partition's sampling interval, its extent along the other dimensions,
and its element dtype:
bytes_per_sample = itemsize * prod(other dims' sample counts)
n_samples = floor(requested_bytes / bytes_per_sample)
The count is floored so the data never exceeds the request, and is
computed against the partition's smallest step: steps within
sampling_group_tolerance share a partition, so sizing against the median
would let a faster-sampled member overshoot.
To make this a pure metadata operation, the index now records each
patch's element dtype (INDEX_VERSION 5 -> 6) and surfaces it to the
spool relation as a private _dtype column. The leading underscore is
load-bearing: chunk's merge-compatibility grouping compares all
non-private columns, so a public dtype column would raise
CoordMergeError on every merge of patches with differing element types.
dtype is deliberately not a partition key, so a partition may mix
dtypes; the estimate uses np.result_type, matching the upcast assembly
performs.
Spool.chunk_plan(...).params["size"] reports what a size resolved to.
The low_freq_proc recipe now uses the new API instead of hand-computing
bytes per second from a loaded patch.
Three defects in the size feature, each with a regression test that fails without its fix. The bound could be exceeded by up to 1.84x. An output holds the sum of its members' sample counts, which equals span/step + 1 only when the partition sits on one grid; members separated by less than one sample each contribute their own trailing sample, so near-contiguous files whose boundaries miss the grid pack more samples into a span than the grid allows. Divide the length by the partition's measured packing factor, which is exactly 1.0 when members tile the grid. This also supersedes the min-step rationale: packing is measured in units of the step used, so the two cancel and the length works out to n_samples / max_density either way. The comment and design note now say so instead of claiming the median step would overshoot. Element dtype is now resolved per output rather than per partition. An output drawing only from its float32 members really is float32, so claiming the partition-wide upcast both over-sized a chained chunk and made a chunked spool compare unequal to its own materialized twin -- for plain chunking too, not just sizes. Non-finite and array-valued quantities are rejected up front. A NaN magnitude is null, so `chunk(time=nan*MB)` silently merged the whole spool when the user asked for a size cap, and `inf` raised a bare ValueError from the sample-count division. Also: never write NaN into the derived catalog's dtype column (NaN is truthy, so `or ""` let the string "nan" through and poisoned every later np.dtype of that column); cover np.result_type with an int32 + float32 partition, where max-itemsize silently doubles the budget while float32 + float64 cannot tell the two rules apart; and replace a vacuous merge-mode test that passed with the feature absent and a tautological one that restated a literal from the module under test.
e59e6d3 to
6d7d8b4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #833 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 164 164
Lines 17936 18096 +160
==========================================
+ Hits 17936 18096 +160
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
test_dtype_round_trips keyed a dict on source_path, which the SQLite round-trip returns with backslashes on Windows while the summary literal uses forward slashes. It failed every Windows job on both the full and min-deps matrices; normalize the separator before comparing. Nothing about the feature is platform-dependent. Codecov also flagged uncovered lines in the new code, all of them real branches rather than dead ones: a frame with no `dims` column falling back to its complete envelopes, an unknown step on the chunked dimension, an unparseable dtype string, a missing envelope on another dimension, and a numeric coordinate rejecting a dimensionally wrong quantity. Each now has a test. Two defensive branches really were unreachable and are gone rather than pragma'd: get_interval_columns cannot fail in _packing_factor because the caller already read those columns for the same partition, and the density array cannot be empty once the step is validated (now a plain assert, which documents the invariant and executes every call).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_core/test_patch_chunk.py (1)
1065-1072: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the docstring: packing density enforces the bound, not the smallest step.
The docstring states that sizing "must" use the smallest step or the byte target is overshot. The implementation states the opposite in
dascore/utils/chunk_plan.pylines 857-861: the packing factor is measured in units of the chosen step, so the choice cancels, and the smallest step only makes the flooring granularity finer. The PR description records the same correction. Align the test docstring with the implementation so a later reader does not treat the step choice as the safety guard.📝 Proposed docstring fix
def test_mixed_steps_in_one_partition_stay_bounded(self): """ - Sizing must use the partition's smallest step, not its median. - - Steps within `sampling_group_tolerance` share a partition, so a - member sampled faster than the median fits more samples into the - same length and would overshoot the byte target. + A partition mixing sampling steps stays inside the byte target. + + Steps within `sampling_group_tolerance` share a partition, so a + member sampled faster than the others packs more samples into the + same length. The measured packing factor absorbs that density, so + the resolved length still fits the request. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_core/test_patch_chunk.py` around lines 1065 - 1072, Update the docstring of test_mixed_steps_in_one_partition_stay_bounded to state that packing density enforces the byte bound, while the smallest step only provides finer flooring granularity; remove the claim that sizing must use the partition’s smallest step to prevent overshooting.tests/test_io/test_index/test_index_contract.py (1)
197-207: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest multiple patches from the same source.
Line 205 uses summaries with one
source_pathper patch.summaries_to_recordsgroups summaries by source. A regression that storesdtypeon the source instead of on each patch can pass this test.Add two summaries with the same
source_pathand different dtypes. Assert their dtype values separately bysource_patch_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_io/test_index/test_index_contract.py` around lines 197 - 207, Update test_dtype_round_trips to include two summaries sharing one source_path but having different dtypes, then assert each returned dtype by its source_patch_id rather than only by path. Preserve path normalization for other entries and ensure the assertions distinguish both patches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_io/test_index/test_index_contract.py`:
- Around line 209-216: Update test_dtype_is_private_in_flat_relation to
construct or obtain an explicitly index-backed spool, then retrieve its contents
through the backend/indexer get_contents path. Preserve the assertions that the
public dtype column is absent and _dtype contains the patch data dtype, ensuring
the test exercises the index backend’s rename behavior.
---
Nitpick comments:
In `@tests/test_core/test_patch_chunk.py`:
- Around line 1065-1072: Update the docstring of
test_mixed_steps_in_one_partition_stay_bounded to state that packing density
enforces the byte bound, while the smallest step only provides finer flooring
granularity; remove the claim that sizing must use the partition’s smallest step
to prevent overshooting.
In `@tests/test_io/test_index/test_index_contract.py`:
- Around line 197-207: Update test_dtype_round_trips to include two summaries
sharing one source_path but having different dtypes, then assert each returned
dtype by its source_patch_id rather than only by path. Preserve path
normalization for other entries and ensure the assertions distinguish both
patches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01e63a0b-fa7a-45fa-8002-8421f31b5aed
📒 Files selected for processing (15)
dascore/core/spool.pydascore/io/index/backend.pydascore/io/index/catalog.pydascore/io/index/indexer.pydascore/io/index/ingest.pydascore/io/index/planned.pydascore/units.pydascore/utils/chunk_plan.pydocs/changelog.qmddocs/notes/spool_chunking.qmddocs/tutorial/spool.qmdtests/test_core/test_patch_chunk.pytests/test_io/test_index/test_index_contract.pytests/test_units.pytests/test_utils/test_chunk.py
🚧 Files skipped from review as they are similar to previous changes (10)
- dascore/core/spool.py
- dascore/io/index/catalog.py
- docs/tutorial/spool.qmd
- dascore/io/index/ingest.py
- dascore/io/index/indexer.py
- docs/notes/spool_chunking.qmd
- dascore/io/index/backend.py
- dascore/io/index/planned.py
- dascore/units.py
- docs/changelog.qmd
| def test_dtype_is_private_in_flat_relation(self): | ||
| """The spool sees `_dtype`, never a public `dtype` column.""" | ||
| import dascore as dc | ||
|
|
||
| spool = dc.get_example_spool("random_das") | ||
| df = spool.get_contents() | ||
| assert "dtype" not in df.columns | ||
| assert set(df["_dtype"]) == {str(spool[0].data.dtype)} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline dascore/examples.py --match get_example_spool --view expanded
rg -n -C 5 'random_das|EXAMPLE_SPOOLS' dascore/examples.py
ast-grep outline dascore/io/index/indexer.py --match get_contents --view expanded
rg -n -C 5 'get_contents\(' tests/test_io/test_index/test_index_contract.pyRepository: DASDAE/dascore
Length of output: 4155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== examples.py random_spool implementation =="
sed -n '555,580p' dascore/examples.py
echo
echo "== dascore spool/base contract around spool and get_contents =="
rg -n -C 4 'class .*Spool|def get_contents|def _get|backend|Indexer' dascore test -g '*.py' | head -n 200
echo
echo "== get_contents call sites in index tests =="
cat -n tests/test_io/test_index/test_index_contract.py | sed -n '190,235p'
echo
echo "== index-related files =="
git ls-files | rg 'dascore/io/index|test_index_contract'Repository: DASDAE/dascore
Length of output: 12641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate spool files =="
git ls-files | rg 'spool|get_contents|indexer|index_contract'
echo
echo "== exact occurrences of get_contents and backend in relevant Python files =="
rg -n -C 6 'def get_contents|backend=|backend|class .*Spool|Indexer' dascore tests/test_io/test_index/test_index_contract.py tests -g '*.py' | sed -n '1,260p'
echo
echo "== dascore/examples.py dc attribute definitions =="
rg -n -C 4 'class Dascore|class .*BaseSpool|spool =|BaseSpool' dascore examples tests -g '*.py' | sed -n '1,220p'
echo
echo "== index contract around dtype tests =="
cat -n tests/test_io/test_index/test_index_contract.py | sed -n '1,260p'Repository: DASDAE/dascore
Length of output: 1914
Use the index-backed spool for this _dtype contract.
dc.get_example_spool("random_das") returns dc.spool([random patches]); this test does not exercise the index backend’s _dtype rename path. Use an explicitly index-backed spool with backend/Indexer.get_contents or a corresponding example that is known to be index-backed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_io/test_index/test_index_contract.py` around lines 209 - 216,
Update test_dtype_is_private_in_flat_relation to construct or obtain an
explicitly index-backed spool, then retrieve its contents through the
backend/indexer get_contents path. Preserve the assertions that the public dtype
column is absent and _dtype contains the patch data dtype, ensuring the test
exercises the index backend’s rename behavior.
|
✅ Documentation built: |
# Conflicts: # docs/changelog.qmd
1 participant
Description
Adds a data-size chunk length so a memory budget can be stated directly:
Each output patch's data array is then at most 25 MB.
docs/recipes/low_freq_proc.qmdhand-rolled exactly this (pa.data.nbytes / pa.seconds→ a length in seconds, which needs a loaded patch and its dtype); that recipe now uses the new API.The same change closes a nearby gap:
Spool.chunkpreviously rejected every pint quantity, sochunk(time=10 * dc.units.s)raisedNotImplementedErrorfromto_timedelta64's singledispatch. Unit-bearing lengths now work too, andoverlapaccepts both forms.How a size resolves
Per partition, since the conversion needs that partition's sampling interval, its extent along the other dimensions, and its element dtype:
Three decisions worth reviewing:
get_middle_value's median. Steps withinconfig.sampling_group_tolerance(5%) share a partition, so sizing against the median lets a faster-sampled member fit more samples into the length and overshoot the byte bound. This makes the bound conservative instead of occasionally violated — and deterministic rather than flaky.np.result_type, matching the upcastpatch_assemblyperforms. dtype is deliberately not a partition key (making it one would change which patches merge), so a partition may legitimately mix element types.result_typeis also strictly more accurate than max-itemsize:int32 + float32 → float64, itemsize 8 rather than 4.Index schema
Chunking must stay a pure metadata operation, so the element dtype comes from the index rather than from loading a patch.
PatchSummary.dtypealready existed andscan_to_dfexposed it, butpatch_record()dropped it on the way in. It is now stored (INDEX_VERSION5 → 6) and reaches the planner as a private_dtypecolumn.The leading underscore is load-bearing, not cosmetic:
_police_columnsskips_-prefixed columns and nothing else protects it, so a publicdtypecolumn would raiseCoordMergeErroron every merge of patches with differing element types. There is a comment at the rename site saying so.dtypeis also added toRESERVED_ATTR_COLUMNSso a patch attr of that name warns-and-drops instead of colliding.The schema is unreleased (not even a beta), so the version bump costs only a local index rebuild.
Units
pint models
byte = 8 * bitwithbitdimensionless, sois_compatible_with("byte")isTruefor50%,1 strain, and any bare dimensionless quantity.is_data_sizetherefore tests the base unit directly.get_byte_countconverts with.to("byte").magnitudeand never throughto_float, whose fallback isfloat(obj)— pint converts a dimensionless quantity to base units, soto_float(25 * MB)returns2e8, the count in bits. A test pins that 8× difference so the two paths cannot be collapsed. (to_float's own quantity handling is tightened separately in the follow-up PR.)Verification
Beyond the suite, checked against real data:
max nbytes 1999800 ≤ 2 MBKnown limitation
chunk(dim=...).chunk(same_dim=...)mis-assembles one patch, which breaks this bound in that specific chain. That is a pre-existing bug, filed as #832 — it reproduces on unmodifieddevwith plain numeric lengths and has nothing to do with sizes or units.TestChainedChunk::test_size_then_sizetherefore asserts on the plan (which is correct and is what this PR owns) and references #832; tighten it to assertnbytesonce that is fixed.Changelog
Spool.chunkaccepts a quantity as the chunk length — a coordinate-unit length (chunk(time=10 * dc.units.s)) or a data size (chunk(time=25 * dc.units.megabytes)) bounding each output patch's array.overlapaccepts both, andchunk_plan(...).params["size"]reports what a size resolved to.dtypeis reserved and stays unindexed. Indexes built by an earlier unreleased version must be rebuilt, which the new index format already requires.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Documentation