Merge master into dev - #815
Conversation
Brings master through #810 into dev. Most conflicts are dev's rewrites (spool unification, attr/coord decoupling) meeting master's later work on the same code; dev's design wins where the two disagree. Notable resolutions: - #777 (Patch.from_parts and the reconcile-twice fast paths): dropped. Dev's constructor does no coord/attr reconciliation, so the cost these avoid no longer exists; measured, from_parts saves 4us per patch, which is 0.02% of pass_filter and 0.05% of differentiate. Master's call sites in filter, strain, differentiate and rolling are restored to dev's constructor. Note this drops from_parts, which was public in v0.1.20. - #787 (change_length validation): ported onto dev's implementation. - #804 (conflict validation): kept in combine_patch_attrs. Dev already validated conflict in build_chunk_plan, so that inline check now uses the shared validate_conflict rather than duplicating it. - #803 (snap_coords): superseded by dev's segmented-coordinate merge, which already honors snap_coords and tolerance. - #774 (DASVader references): master dropped an over-eager rejection of anonymous refs while dev added a low-level fallback for refs the high-level lookup can't resolve. Both are kept; they are complementary and each branch has a test for its case. - #810 (range queries on interval columns): both branches reject spool.select(time_min=...); dev's unknown-name check runs first, so its message is the one asserted. - rolling: master's dataclass conversion kept, but its attrs handling passes coords= to attrs.update, which dev's PatchAttrs rejects, so dev's history handling is kept and _new_patch builds the output patch. Attrs stay entirely decoupled from coords, so master's tests and helpers which keep the two in sync are dropped: TestFromParts, TestAttrsConformTo, TestAttrsCoordsInvariant, test_attrs_conform_to_coords, PatchAttrs._conform_to, and test_conflicting_attrs_coords_raises (which on dev was decorated as a fixture and never ran). Master's ChunkManager-based tests go with the class dev deleted.
d-chambers
added
documentation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 14 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds import and construction benchmarks, strengthens coordinate and query validation, improves DASVader compatibility handling, makes processing NaN-aware, refactors rolling execution, centralizes conflict validation, expands regression tests, and corrects documentation. ChangesBenchmark coverage
Coordinate and patch validation
DASVader reference compatibility
NaN-aware processing
Rolling execution and metadata
Conflict argument validation
Pandas range queries
Documentation corrections
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
benchmark
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #815 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17851 17879 +28
=========================================
+ Hits 17851 17879 +28
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/utils/pd.py (1)
348-374: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize
Ellipsisopen bounds toNone.The range-query contract accepts both
Noneand...as open bounds. This helper only preservesNone.A query such as
filter_df(df, time=(..., cutoff))passesEllipsistoto_datetime64orto_timedelta64. Those converters do not supportEllipsis, so valid open-ended temporal queries fail.Normalize
EllipsistoNonebefore conversion. Add regression coverage for datetime and timedelta queries with....Proposed fix
- Unbounded (None) ends are left alone; converting them would produce NaT, + Unbounded (`None` or `...`) ends are normalized to `None`; converting them + would produce NaT, which compares False against everything and would silently empty the query. """ - return tuple(None if x is None else func(x) for x in range_tuple) + return tuple(None if x is None or x is ... else func(x) for x in range_tuple)🤖 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/pd.py` around lines 348 - 374, Update _convert_range_bounds to treat Ellipsis the same as None, converting both open-bound markers to None while applying func only to actual values. Add regression coverage through _convert_times or filter_df for datetime and timedelta ranges using ... as an open bound, preserving conversion of finite bounds.
🧹 Nitpick comments (1)
tests/test_core/test_patch.py (1)
394-400: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the full patch-adoption contract.
The test checks only
out.coordsandout.shape. A regression could return the correct shape with incorrect data or attrs and still pass. Add assertions forout.dataand a distinctive attr fromother. If attrs are not part of the contract, narrow the docstring.Proposed test strengthening
- other = random_patch.decimate(time=2) + other = random_patch.decimate(time=2).update_attrs(tag="adopted") out = random_patch.new(data=other) + assert np.array_equal(out.data, other.data) assert out.coords == other.coords + assert out.attrs.tag == other.attrs.tag assert out.shape == other.shape🤖 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.py` around lines 394 - 400, Strengthen test_new_from_patch_takes_over_metadata by asserting that out.data matches other.data and that a distinctive attribute from other.attrs is preserved. Keep the existing coordinate and shape assertions, and update the docstring only if attribute adoption is not intended to be part of the contract.
🤖 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 `@benchmarks/readme.md`:
- Line 31: Update the benchmark description for test_import_benchmarks.py to
hyphenate the compound modifier, changing “third party dependencies” to
“third-party dependencies.”
In `@benchmarks/test_patch_benchmarks.py`:
- Around line 224-227: Update test_from_parts to stop calling the removed
dc.Patch.from_parts method; use the supported dc.Patch constructor path already
exercised by test_patch_init, or remove this benchmark case while preserving
valid construction benchmarking.
In `@dascore/io/dasvader/utils.py`:
- Around line 78-90: Update _dereference to catch both KeyError and ValueError
from the initial h5[value] lookup, allowing invalid references to reach the
low-level dereference fallback and _raise_legacy_ref_error. Preserve the
fallback’s existing behavior, and chain the original lookup exception when
raising DASVaderCompatibilityError so its traceback context is retained.
In `@dascore/proc/basic.py`:
- Around line 362-372: The bit normalization branch around norm must avoid
signed-integer overflow when computing the divisor, preserving the sign of the
minimum representable value; convert signed integer data to the floating result
dtype before applying np.abs or use an equivalent overflow-free sign
calculation. In dascore/proc/basic.py lines 362-372, update the bit
normalization logic accordingly. In tests/test_proc/test_basic.py lines 245-256,
add a regression case using np.iinfo(np.int8).min and assert that normalization
returns -1.
---
Outside diff comments:
In `@dascore/utils/pd.py`:
- Around line 348-374: Update _convert_range_bounds to treat Ellipsis the same
as None, converting both open-bound markers to None while applying func only to
actual values. Add regression coverage through _convert_times or filter_df for
datetime and timedelta ranges using ... as an open bound, preserving conversion
of finite bounds.
---
Nitpick comments:
In `@tests/test_core/test_patch.py`:
- Around line 394-400: Strengthen test_new_from_patch_takes_over_metadata by
asserting that out.data matches other.data and that a distinctive attribute from
other.attrs is preserved. Keep the existing coordinate and shape assertions, and
update the docstring only if attribute adoption is not intended to be part of
the contract.
🪄 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: 836c67bb-fd1a-4b43-bfb1-f654f9eede17
📒 Files selected for processing (25)
benchmarks/readme.mdbenchmarks/test_import_benchmarks.pybenchmarks/test_patch_benchmarks.pydascore/core/coords.pydascore/io/dasvader/core.pydascore/io/dasvader/utils.pydascore/proc/basic.pydascore/proc/rolling.pydascore/transform/fbe.pydascore/utils/attrs.pydascore/utils/chunk_plan.pydascore/utils/pd.pydocs/recipes/smoothing.qmddocs/tutorial/patch.qmddocs/tutorial/processing.qmddocs/tutorial/spool.qmdtests/test_core/test_coords.pytests/test_core/test_patch.pytests/test_core/test_patch_chunk.pytests/test_io/test_dasvader/test_dasvader.pytests/test_proc/test_basic.pytests/test_proc/test_rolling.pytests/test_utils/test_attrs_utils.pytests/test_utils/test_chunk.pytests/test_utils/test_pd.py
| - `test_io_benchmarks.py` - File I/O operations benchmarks | ||
| - `test_spool_benchmarks.py` - Spool chunking and selection benchmarks | ||
| - `test_lookup_benchmarks.py` - In-memory lookups on hot paths (format resolution, remote-cache and IO handle resolution, repeat spool access). These are deliberately small: a change of a few microseconds per lookup is invisible in the end-to-end benchmarks above, because one file read costs far more than the lookups it makes. | ||
| - `test_import_benchmarks.py` - Import benchmarks (dascore's own modules; third party dependencies stay warm) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
Change third party dependencies to third-party dependencies.
🧰 Tools
🪛 LanguageTool
[grammar] ~31-~31: Use a hyphen to join words.
Context: ...benchmarks (dascore's own modules; third party dependencies stay warm) Each benc...
(QB_NEW_EN_HYPHEN)
🤖 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 `@benchmarks/readme.md` at line 31, Update the benchmark description for
test_import_benchmarks.py to hyphenate the compound modifier, changing “third
party dependencies” to “third-party dependencies.”
Source: Linters/SAST tools
| def _dereference(h5, value, field_name: str): | ||
| """Resolve an HDF5 reference, rejecting legacy anonymous DASVader refs.""" | ||
| """Resolve an HDF5 reference or raise a clear compatibility error.""" | ||
| if not isinstance(value, Reference): | ||
| return value | ||
| if h5r.get_name(value, h5.id) is None: | ||
| _raise_legacy_ref_error(h5, field_name) | ||
| try: | ||
| return h5[value] | ||
| except KeyError: | ||
| return h5py.Dataset(dereference(value, h5.id)) | ||
| # The high-level lookup fails for some references HDF5 can still | ||
| # resolve directly, so try that before giving up. | ||
| try: | ||
| return h5py.Dataset(dereference(value, h5.id)) | ||
| except Exception: | ||
| _raise_legacy_ref_error(h5, field_name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does h5py Group.__getitem__ raise ValueError for an unresolvable object reference?
💡 Result:
Yes, h5py Group.getitem raises a ValueError when provided with an unresolvable or "null" object reference [1][2]. In the implementation of Group.getitem, h5py checks if the provided key is an instance of h5r.Reference [1]. If it is, the library attempts to dereference it using h5r.dereference [1]. If the dereferencing process returns None (which occurs for zero-filled or null references), h5py explicitly raises a ValueError with the message "Invalid HDF5 object reference" [1][3][2]. This behavior is specific to object references. In contrast, when accessing group members by string path, h5py typically raises a KeyError if the object does not exist [4][5].
Citations:
- 1: https://github.com/h5py/h5py/blob/master/h5py/_hl/group.py
- 2: https://github.com/h5py/h5py/blob/e0dd0ff5461b7192e58e996f865544e60a7ba11a/h5py/_hl/group.py
- 3: Can't load object references saved by C++ h5py/h5py#2123
- 4: https://docs.h5py.org/en/latest/high/group.html
- 5: https://docs.h5py.org/en/3.11.0/high/group.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching utils.py:\n'
fd 'utils.py$' . || true
printf '\nTarget file outline:\n'
ast-grep outline dascore/io/dasvader/utils.py --view expanded || true
printf '\nRelevant portion of dascore/io/dasvader/utils.py:\n'
cat -n dascore/io/dasvader/utils.py | sed -n '1,140p'
printf '\nUsages of _dereference and DASVaderCompatibilityError:\n'
rg -n "_dereference|DASVaderCompatibilityError|h5yp|Reference" dascore/io/dasvader/utils.py dascore io 2>/dev/null | head -200 || true
printf '\nPython module availability and h5py reference resolution behavior probe:\n'
python3 - <<'PY'
import inspect, struct, tempfile, os
try:
import h5py, h5py.h5r as h5r
except Exception as exc:
print(f"h5py import error: {type(exc).__name__}: {exc}")
raise SystemExit(0)
print("h5py version:", getattr(h5py, "__version__", "unknown"))
src = inspect.getsource(h5py.h5r.Reference)
print(f"\nh5py.h5r.Reference.source:\n{src[:3000]}")
src = inspect.getsource(h5py.Group.__getitem__)
print(f"\nh5py.Group.__getitem__.source:\n{src}")
def make_zero_ref():
return b'0'*48
for dtype in ("u8", "i8", "<u8", ">u8", "<i8", ">i8", None):
arr = make_zero_ref()
buf = arr[:]
if dtype is None or dtype.startswith("<"):
buf = buf.swapbytes()
print(dtype, repr(buf), "starts with zero bytes:", buf[:4] == b'\x00\0\0\0')
with tempfile.NamedTemporaryFile(suffix=".h5", delete=False) as f:
path = f.name
try:
with h5py.File(path, "w") as f:
f.attrs["zero_ref"] = make_zero_ref()
f.attrs["one_ref"] = make_zero_ref()
f.attrs["one_ref"][0] = b"\x0F"[0] if isinstance(make_zero_ref(), bytes) else 15
with h5py.File(path, "r") as f:
zero = f.attrs["zero_ref"]
one = f.attrs["one_ref"]
print("\nReading attrs:", zero, one)
print("is Reference?", isinstance(zero, h5r.Reference), isinstance(one, h5r.Reference))
for label, ref in [("zero_ref", zero), ("one_ref", one)]:
try:
val = f[ref]
print("getitem succeed", label, type(val))
except Exception as exc:
print("getitem", label, type(exc).__name__, str(exc))
try:
res = h5r.dereference(ref, f.id)
print("h5r.dereference raw", label, res, type(res))
except Exception as exc:
print("h5r.dereference", label, type(exc).__name__, str(exc))
finally:
os.unlink(path)
PYRepository: DASDAE/dascore
Length of output: 9980
🌐 Web query:
h5py _hl group.py Group.__getitem__ Reference None ValueError Invalid HDF5 object reference
💡 Result:
The ValueError "Invalid HDF5 object reference" in h5py is raised within the Group.getitem method when an attempt to dereference an HDF5 object reference fails [1][2]. Specifically, in h5py/_hl/group.py, when a user attempts to access an item in a group by passing an object reference (as opposed to a string path), h5py calls h5r.dereference [1][3]. If the result of this dereference is None—indicating that the reference points to a non-existent, invalid, or inaccessible object—h5py explicitly raises this ValueError [1][2]. This error typically indicates one of the following issues: 1. Broken Links: The reference may point to an object that has been deleted, moved, or is part of a broken external link [4]. 2. Corrupted File or Reference: The HDF5 file or the specific reference data might be corrupted, leading to an undefined object address [5]. 3. Incorrect Reference Handling: The reference might have been created or stored incorrectly, such as when using external C++ code to generate HDF5 files where pointers or reference arrays are not managed correctly [2]. If you encounter this error while accessing data, you can investigate by using command-line HDF5 tools like h5ls or h5dump to inspect the structure of the file and verify if the target object exists or if links are broken [4]. Additionally, ensure that any references were generated in a manner compatible with the HDF5 library's requirements [2].
Citations:
- 1: https://github.com/h5py/h5py/blob/e0dd0ff5461b7192e58e996f865544e60a7ba11a/h5py/_hl/group.py
- 2: Can't load object references saved by C++ h5py/h5py#2123
- 3: https://github.com/h5py/h5py/blob/master/h5py/_hl/group.py
- 4: https://groups.google.com/g/h5py/c/CZFa3ap47YE
- 5: Segmentation fault loading a crafted model h5py/h5py#1183
Catch ValueError too, so the fallback and clear error always trigger.
_dereference only catches KeyError from h5[value]. h5py raises ValueError with message Invalid HDF5 object reference for unresolvable object references, so those cases skip the low-level fallback and expose a raw exception instead. Add ValueError to the caught exception on line 84.
🐛 Proposed fix
try:
return h5[value]
- except KeyError:
+ except (KeyError, ValueError):
# The high-level lookup fails for some references HDF5 can still
# resolve directly, so try that before giving up.
try:
return h5py.Dataset(dereference(value, h5.id))
except Exception:
_raise_legacy_ref_error(h5, field_name)Keep the narrow Exception catch in the low-level fallback only if it must absorb unrelated resource failures, but chain the original error so DASVaderCompatibilityError preserves traceback context.
📝 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 _dereference(h5, value, field_name: str): | |
| """Resolve an HDF5 reference, rejecting legacy anonymous DASVader refs.""" | |
| """Resolve an HDF5 reference or raise a clear compatibility error.""" | |
| if not isinstance(value, Reference): | |
| return value | |
| if h5r.get_name(value, h5.id) is None: | |
| _raise_legacy_ref_error(h5, field_name) | |
| try: | |
| return h5[value] | |
| except KeyError: | |
| return h5py.Dataset(dereference(value, h5.id)) | |
| # The high-level lookup fails for some references HDF5 can still | |
| # resolve directly, so try that before giving up. | |
| try: | |
| return h5py.Dataset(dereference(value, h5.id)) | |
| except Exception: | |
| _raise_legacy_ref_error(h5, field_name) | |
| def _dereference(h5, value, field_name: str): | |
| """Resolve an HDF5 reference or raise a clear compatibility error.""" | |
| if not isinstance(value, Reference): | |
| return value | |
| try: | |
| return h5[value] | |
| except (KeyError, ValueError): | |
| # The high-level lookup fails for some references HDF5 can still | |
| # resolve directly, so try that before giving up. | |
| try: | |
| return h5py.Dataset(dereference(value, h5.id)) | |
| except Exception: | |
| _raise_legacy_ref_error(h5, field_name) |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 89-89: Do not catch blind exception: Exception
(BLE001)
🤖 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/io/dasvader/utils.py` around lines 78 - 90, Update _dereference to
catch both KeyError and ValueError from the initial h5[value] lookup, allowing
invalid references to reach the low-level dereference fallback and
_raise_legacy_ref_error. Preserve the fallback’s existing behavior, and chain
the original lookup exception when raising DASVaderCompatibilityError so its
traceback context is retained.
| elif norm == "bit": | ||
| pass | ||
| divisor = np.abs(data) | ||
| else: | ||
| msg = ( | ||
| f"Norm value of {norm} is not supported. " | ||
| f"Supported values are {('l1', 'l2', 'max', 'bit')}" | ||
| ) | ||
| raise ValueError(msg) | ||
| if norm == "bit": | ||
| new_data = np.divide( | ||
| data, np.abs(data), out=np.zeros_like(data), where=np.abs(data) != 0 | ||
| ) | ||
| else: | ||
| expanded_norm = np.expand_dims(norm_values, axis=axis) | ||
| new_data = np.divide( | ||
| data, expanded_norm, out=np.zeros_like(data), where=expanded_norm != 0 | ||
| ) | ||
| # A zero divisor means there is nothing but zeros and nulls to scale, so | ||
| # divide those by one; the zeros stay zero and the nulls stay null. | ||
| new_data = data / np.where(divisor == 0, 1, divisor) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import numpy as np
value = np.array([np.iinfo(np.int8).min], dtype=np.int8)
assert (value / np.abs(value))[0] == 1.0
PYRepository: DASDAE/dascore
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^dascore/proc/basic\.py$|^tests/test_proc/test_basic\.py$)' || true
echo
echo "== basic.py normalize implementation around bit norm =="
sed -n '320,400p' dascore/proc/basic.py
echo
echo "== test_basic.py integer normalization tests =="
sed -n '220,275p' tests/test_proc/test_basic.py
echo
echo "== search for normalize signature and usage =="
rg -n "def normalize|normalize\\(|norm =|\"bit\"|'bit'" dascore tests/test_proc/test_basic.py -SRepository: DASDAE/dascore
Length of output: 9655
Preserve the sign of signed-integer minimum values in bit normalization.
np.abs cannot represent the absolute value of a signed dtype minimum. For example, np.int8(-128) / np.abs(np.int8(-128)) produces +1, not -1.
dascore/proc/basic.py#L362-L372: convert signed integer data to the floating result dtype beforenp.abs, or use an overflow-free sign calculation.tests/test_proc/test_basic.py#L245-256: add abitregression case fornp.iinfo(np.int8).minand assert that its result is-1.
📍 Affects 2 files
dascore/proc/basic.py#L362-L372(this comment)tests/test_proc/test_basic.py#L245-L256
🤖 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/proc/basic.py` around lines 362 - 372, The bit normalization branch
around norm must avoid signed-integer overflow when computing the divisor,
preserving the sign of the minimum representable value; convert signed integer
data to the floating result dtype before applying np.abs or use an equivalent
overflow-free sign calculation. In dascore/proc/basic.py lines 362-372, update
the bit normalization logic accordingly. In tests/test_proc/test_basic.py lines
245-256, add a regression case using np.iinfo(np.int8).min and assert that
normalization returns -1.
The benchmark went with Patch.from_parts, which this merge removes.
|
✅ Documentation built: |
Merging this PR will degrade performance by 44.17%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_standardize |
19.4 ms | 34.8 ms | -44.17% |
| 🆕 | test_rolling_mean_full_call |
N/A | 55.2 ms | N/A |
| 🆕 | test_new_data_only |
N/A | 201.6 µs | N/A |
| 🆕 | test_new_with_coords |
N/A | 206.7 µs | N/A |
| 🆕 | test_new_with_coords_and_attrs |
N/A | 203.2 µs | N/A |
| 🆕 | test_patch_init |
N/A | 201.9 µs | N/A |
| 🆕 | test_reimport_dascore |
N/A | 127.4 ms | N/A |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing merge-master-to-dev (37a0345) with dev (0afc948)
1 participant
Description
Merges
master(through #810) intodev, the last of the three blockers for cutting a beta fromdev.Eleven master commits come across: #774, #771, #775, #777, #789, #787, #793, #806, #805, #808, #810. Seventeen files conflicted, almost all of them dev's rewrites (spool unification, attr/coord decoupling) meeting master's later work on the same code. Dev's design wins wherever the two disagree.
Resolutions worth reviewing
#777 (
Patch.from_parts) — dropped. It exists to skip recomputing the coordinate summaries stored on attrs, and dev's decoupling means attrs no longer carry those, so the cost it avoids is gone. Measured on dev it saves ~4 µs per patch, which is 0.02% ofpass_filterand 0.05% ofdifferentiate— noise. Master's call sites infilter.py,strain.py,differentiate.pyandrolling.pyare restored to dev's constructor. This does dropfrom_parts, which was public in v0.1.20 — deliberate, since keeping it on dev would mean carrying public API that buys nothing.rolling.pyis a hybrid. Master's_PatchRollerInfodataclass conversion is kept (independent perf work, compatible), but master's attrs handling passescoords=toattrs.update(), which dev'sPatchAttrsexplicitly rejects. So dev's history handling stays and_new_patchbuilds the output patch._dereferenceunions both branches. Master's #774 removed an over-eager rejection of anonymous references; dev separately added a low-level fallback for references the high-level lookup can't resolve. These fix different problems, so both are kept — each branch has a test for its case, and either version alone fails the other's.#787 (
change_lengthvalidation) is ported onto dev's implementation. #804's validation stays incombine_patch_attrs; dev already validatedconflictinbuild_chunk_plan, so that inline check now calls the sharedvalidate_conflictinstead of duplicating it. #803 is superseded by dev's segmented-coordinate merge, which already honorssnap_coordsandtolerance.Tests removed
Attrs stay entirely decoupled from coords, so master's tests and helpers that keep the two in sync are dropped:
TestFromParts,TestAttrsConformTo,TestAttrsCoordsInvariant,test_attrs_conform_to_coords, andPatchAttrs._conform_to. Master'sChunkManager-based tests go with the class dev deleted.Also removed
test_conflicting_attrs_coords_raises. Worth knowing: on dev that function is decorated@pytest.fixture, so it has never run — which was masking that dev accepts flat coord-like attrs (a patch can carryattrs.distance_min == 1000beside a real coord min of0.16). That is the intended decoupled behavior, but a sweep for othertest_-named fixtures is probably worthwhile.One master test was adjusted rather than dropped: #810's
test_spool_select_open_bound_on_interval_column. Dev already rejectsspool.select(time_min=...)with its own unknown-name check, which runs first, so the behavior #810 fixed is present and only the asserted message differs.Changelog
none
Checklist
I have (if applicable):
dev.)Summary by CodeRabbit
Bug Fixes
Documentation
Performance