Merge master into dev - #769
Conversation
* Raise CoordError instead of assert for non-1D coord operations Several coordinate operations that only support 1D coords guarded their input with `assert`, which raises a bare AssertionError and is stripped entirely under `python -O` (so the check silently vanishes in optimized runs). Convert the user-reachable ones -- select-by-sample-array, align_to, get_sample_count, CoordPartial.change_length, and CoordRange construction -- to raise CoordError. Genuine internal invariants that are impossible by construction (CoordRange.change_length) stay as asserts. Add tests covering each new error path. * Extend assert->raise cleanup to proc, viz, and wav IO Apply the same treatment repo-wide to user-reachable asserts that validate caller input, converting them to ParameterError: - proc/taper: taper window must be a length-2 sequence - proc/detrend: dim must be in the patch - proc/correlate: patch must be 2D - viz/map_fiber: x/y/color must be existing coords; scale_type and scale validated - io/wav: only single-patch spools can be written to wav Internal invariants (impossible-by-construction shape/postcondition checks, binary-format parser consistency) are left as asserts. Adds tests for every new error path; the five changed modules keep 100% line coverage.
* fixed decibel scaling factore to 20 (was 10) * fixed test to match new decibel factor
* implemented gap-sensistive waterfall plot * added 3 more tests * refactor gap_detection and mesh-coordinates; handle datetime64 natively
…ev-20260724 # Conflicts: # dascore/io/wav/core.py # dascore/proc/correlate.py # tests/test_core/test_coords.py # tests/test_io/test_wav/test_wav.py
📝 WalkthroughWalkthroughThe PR updates release instructions and CI actions, preserves objects on coordinate no-ops, replaces assertions with explicit errors, corrects FBE dB scaling, and adds gap-aware waterfall rendering with utilities, tests, and benchmarks. ChangesDASCore and repository workflow changes
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #769 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 163 164 +1
Lines 17221 17357 +136
==========================================
+ Hits 17221 17357 +136
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:
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/test_viz/test_waterfall.py (1)
68-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: consolidate the two near-identical gap fixtures.
distance_gap_patchandtime_gap_patchare structurally identical apart from the coordinate name. Could be collapsed into one parametrized fixture (e.g. parametrize ondim) to avoid duplicated maintenance.🤖 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_viz/test_waterfall.py` around lines 68 - 84, Consolidate the duplicated distance_gap_patch and time_gap_patch fixtures into a single parametrized fixture keyed by the coordinate dimension, such as “distance” or “time.” Preserve the existing gap construction and return shape so each test continues receiving the updated patch and split index.tests/test_utils/test_gaps.py (1)
75-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: add datetime/timedelta cases to
TestIsMonotonicAndFinite.
is_monotonic_and_finiteis used directly on datetime64/timedelta64 coordinate arrays indascore/viz/waterfall.py's renderer-selection logic, but this suite only exercises plain numeric lists. Direct coverage here (rather than only indirectly viatest_viz/test_waterfall.py) would pin down the datetime/timedelta contract in isolation.🤖 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_utils/test_gaps.py` around lines 75 - 83, Extend TestIsMonotonicAndFinite with direct datetime64 and timedelta64 cases for is_monotonic_and_finite, covering valid monotonic finite arrays and invalid or nonfinite values as supported by the function’s contract. Keep the existing numeric tests unchanged and place the new coverage alongside test_valid and test_invalid.dascore/utils/gaps.py (1)
87-104: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider vectorizing the gap-edge construction loop.
This loop runs once per element of the irregular coordinate (O(N) pure Python) every time
_plot_with_meshrenders. For large irregular coordinate arrays with few gaps, this could become a real cost on a plotting hot path. A vectorized approach — compute the defaultvalues[:-1] + diffs/2edges array, then insert the two gap-boundary edges at gap positions using a cumulative-index technique (similar to the one already used in_insert_gap_bandsindascore/viz/waterfall.py) — would avoid the per-element Python loop while preserving identical output.♻️ Sketch of a vectorized approach
# default midpoint edges, vectorized mid_edges = values[:-1] + diffs / 2 # at gap positions, override with signed_step-based boundaries using # an insertion technique analogous to _insert_gap_bands' cumsum trick, # instead of the per-index Python loop.🤖 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/gaps.py` around lines 87 - 104, Replace the per-element edge construction loop in the surrounding gap-edge function with a vectorized approach: compute the default midpoint edges from values and diffs, then insert or override the two signed_step-based boundary edges for gap positions using a cumulative-index technique like _insert_gap_bands. Preserve first_step, last_step, edge ordering, and identical output for both gap and non-gap intervals.
🤖 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 @.agents/skills/draft-release/SKILL.md:
- Around line 96-97: Update the reverted PR example in the release skill
documentation so both `#126` and `#127` include their corresponding PR links, while
preserving the existing “Reverted (no net change)” wording and formatting.
- Around line 54-59: Update the reverted-pair guidance in the release drafting
instructions to require verifying the combined diff or resulting behavior before
omitting either PR from the main sections. Keep title/body wording as an
identification signal only, and list the pair under “Reverted (no net change)”
only when the check confirms no net user-facing change.
In @.github/workflows/build_deploy_master_docs.yaml:
- Around line 34-37: Disable persisted checkout credentials by adding
persist-credentials: false to the checkout steps in
.github/workflows/build_deploy_master_docs.yaml (34-37),
.github/workflows/build_deploy_stable_docs.yaml (29-32),
.github/workflows/get_coverage.yml (13-16), .github/workflows/lint.yml (19-22),
.github/workflows/profile.yml (30-33), both sites in
.github/workflows/run_min_dep_tests.yml (37 and 61-64), both sites in
.github/workflows/runtests.yml (41 and 66-69),
.github/workflows/test_doc_build.yml (18-21), and
.github/workflows/upload_pypi.yml (12-15). Verify the
.github/actions/mamba-install-dascore jobs first and do not change them unless
their later steps do not require authenticated Git.
In `@dascore/core/coords.py`:
- Around line 375-377: Require exactly one dimension in all affected coordinate
APIs by changing the ndim guards in dascore/core/coords.py ranges 375-377,
460-462, 949-951, and 1247-1249 from greater-than-one checks to not-equal-one
checks. Add regression cases in tests/test_core/test_coords.py range 2581-2617
using CoordPartial(shape=()) to verify sample-array selection, alignment,
sample-count calculation, and partial-coordinate resizing reject rank-0
coordinates.
In `@dascore/transform/fbe.py`:
- Around line 44-45: Update the dB documentation in the relevant transform
function to correct “orginal” to “original” and remove the extra closing
parenthesis from the 20 * log10(sqrt(mean(x^2))) formula, keeping the documented
calculation aligned with the implementation.
In `@dascore/viz/map_fiber.py`:
- Around line 24-29: Update the scale validation in the map-fiber configuration
flow to accept only numeric scalars or supported Sequence/np.ndarray values
before calling len, rejecting unsupported objects without leaking TypeError and
preventing strings such as "ab" from passing. Reuse the same scalar predicate in
the scale conversion logic around line 35 so validation and conversion apply
identical type rules.
---
Nitpick comments:
In `@dascore/utils/gaps.py`:
- Around line 87-104: Replace the per-element edge construction loop in the
surrounding gap-edge function with a vectorized approach: compute the default
midpoint edges from values and diffs, then insert or override the two
signed_step-based boundary edges for gap positions using a cumulative-index
technique like _insert_gap_bands. Preserve first_step, last_step, edge ordering,
and identical output for both gap and non-gap intervals.
In `@tests/test_utils/test_gaps.py`:
- Around line 75-83: Extend TestIsMonotonicAndFinite with direct datetime64 and
timedelta64 cases for is_monotonic_and_finite, covering valid monotonic finite
arrays and invalid or nonfinite values as supported by the function’s contract.
Keep the existing numeric tests unchanged and place the new coverage alongside
test_valid and test_invalid.
In `@tests/test_viz/test_waterfall.py`:
- Around line 68-84: Consolidate the duplicated distance_gap_patch and
time_gap_patch fixtures into a single parametrized fixture keyed by the
coordinate dimension, such as “distance” or “time.” Preserve the existing gap
construction and return shape so each test continues receiving the updated patch
and split index.
🪄 Autofix (Beta)
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: cb7e610e-72ce-492a-93d2-d8f86cc0ab3f
📒 Files selected for processing (34)
.agents/skills/draft-release/SKILL.md.github/actions/mamba-install-dascore/action.yml.github/workflows/build_deploy_master_docs.yaml.github/workflows/build_deploy_stable_docs.yaml.github/workflows/get_coverage.yml.github/workflows/lint.yml.github/workflows/profile.yml.github/workflows/run_min_dep_tests.yml.github/workflows/runtests.yml.github/workflows/test_doc_build.yml.github/workflows/upload_pypi.ymlbenchmarks/test_patch_benchmarks.pydascore/core/coordmanager.pydascore/core/coords.pydascore/io/wav/core.pydascore/proc/coords.pydascore/proc/correlate.pydascore/proc/detrend.pydascore/proc/taper.pydascore/transform/fbe.pydascore/utils/gaps.pydascore/viz/map_fiber.pydascore/viz/waterfall.pytests/test_core/test_coordmanager.pytests/test_core/test_coords.pytests/test_io/test_wav/test_wav.pytests/test_proc/test_correlate.pytests/test_proc/test_detrend.pytests/test_proc/test_proc_coords.pytests/test_proc/test_taper.pytests/test_transform/test_fbe.pytests/test_utils/test_gaps.pytests/test_viz/test_map_fiber.pytests/test_viz/test_waterfall.py
| 5. Drop reverted pairs first. If a PR in scope reverts another PR that is also | ||
| in scope (revert PRs usually say "Revert ..." and name the reverted PR or | ||
| commit in the title/body), the two cancel out to no net user-facing change. | ||
| Omit both from the sections and instead list them under a short | ||
| `Reverted (no net change)` note at the end, so the reader knows why those PR | ||
| numbers are absent. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify reverted pairs before omitting them.
A “Revert …” title/body is not sufficient proof that the original PR and revert produce no net user-facing change; reverts can be partial or followed by dependent changes. Require a diff/result check before dropping both PRs from the main sections.
Proposed wording
-5. Drop reverted pairs first. If a PR in scope reverts another PR that is also
- in scope ...
+5. Identify suspected reverted pairs first. Confirm from the PR diff and resulting
+ branch history that the revert fully removes the original user-facing change;
+ only then omit both PRs ...📝 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.
| 5. Drop reverted pairs first. If a PR in scope reverts another PR that is also | |
| in scope (revert PRs usually say "Revert ..." and name the reverted PR or | |
| commit in the title/body), the two cancel out to no net user-facing change. | |
| Omit both from the sections and instead list them under a short | |
| `Reverted (no net change)` note at the end, so the reader knows why those PR | |
| numbers are absent. | |
| 5. Identify suspected reverted pairs first. Confirm from the PR diff and resulting | |
| branch history that the revert fully removes the original user-facing change; | |
| only then omit both PRs from the sections and instead list them under a short | |
| `Reverted (no net change)` note at the end, so the reader knows why those PR | |
| numbers are absent. |
🤖 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 @.agents/skills/draft-release/SKILL.md around lines 54 - 59, Update the
reverted-pair guidance in the release drafting instructions to require verifying
the combined diff or resulting behavior before omitting either PR from the main
sections. Keep title/body wording as an identification signal only, and list the
pair under “Reverted (no net change)” only when the check confirms no net
user-facing change.
|
|
||
| Reverted (no net change): #126 reverted by #127 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include links for reverted PRs too.
The output example omits links for #126 and #127, contradicting the requirement to include a PR link for changelog entries and reducing traceability.
Proposed wording
-Reverted (no net change): `#126` reverted by `#127`
+Reverted (no net change): `#126` (https://github.com/OWNER/REPO/pull/126) reverted by `#127` (https://github.com/OWNER/REPO/pull/127)📝 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.
| Reverted (no net change): #126 reverted by #127 | |
| Reverted (no net change): `#126` (https://github.com/OWNER/REPO/pull/126) reverted by `#127` (https://github.com/OWNER/REPO/pull/127) |
🤖 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 @.agents/skills/draft-release/SKILL.md around lines 96 - 97, Update the
reverted PR example in the release skill documentation so both `#126` and `#127`
include their corresponding PR links, while preserving the existing “Reverted
(no net change)” wording and formatting.
| - uses: actions/checkout@v5 | ||
| with: | ||
| fetch-tags: "true" | ||
| fetch-depth: '0' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- workflow checkout occurrences ---\n'
for f in $(git ls-files '.github/workflows/*'); do
echo "### $f"
nl -ba "$f" | sed -n '1,120p' | grep -n -A4 -B2 'actions/checkout' || true
done
printf '\n--- network_tests context references ---\n'
rg -n "network_tests|persist-credentials|fetch-tags|fetch-depth" .github/workflows || trueRepository: DASDAE/dascore
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow checkout occurrences ---'
find .github/workflows -maxdepth 1 -type f -print | sort | while read -r f; do
echo "### $f"
nl -ba "$f" | sed -n '1,120p' | grep -n -A4 -B2 'actions/checkout' || true
done
printf '%s\n' ''
printf '%s\n' '--- network_tests / persist-credentials / fetch flags references ---'
rg -n "network_tests|persist-credentials|fetch-tags|fetch-depth" .github/workflows || trueRepository: DASDAE/dascore
Length of output: 2701
🌐 Web query:
actions/checkout v5 persist-credentials default true documentation
💡 Result:
In actions/checkout v5, the persist-credentials input parameter defaults to true [1][2]. When set to true (the default), the action configures the GitHub Actions authentication token or SSH key in the local git configuration [2][3]. This allows your workflow scripts to run authenticated git commands (such as git push or git pull) against the repository [2][4]. The action automatically removes these credentials during the post-job cleanup process [2][5]. If you wish to prevent the action from configuring these credentials, you can explicitly set persist-credentials to false in your workflow file [2][3]: - uses: actions/checkout@v5 with: persist-credentials: false
Citations:
- 1: https://git.liteyuki.org/actions/checkout/src/tag/v5.0.1/action.yml
- 2: https://github.com/actions/checkout
- 3: https://github.com/marketplace/actions/checkout
- 4: https://git.liteyuki.org/actions/checkout/raw/tag/v5.0.1/README.md
- 5: https://git.liteyuki.org/actions/checkout/src/branch/releases/v5/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow checkout lines with line numbers ---'
find .github/workflows -maxdepth 1 -type f -print | sort | while read -r f; do
echo "### $f"
grep -n -C 3 'actions/checkout' "$f" || true
done
printf '%s\n' ''
printf '%s\n' '--- YAML references to ghp-token / gh-token / git fetch --tags / auth in docs workflows ---'
rg -n 'ghp-token|gh-token|git fetch|--fetch|fetch tags|mamba-install-dascore|uses: mamba' .github/workflows || true
printf '%s\n' ''
printf '%s\n' '--- file sizes ---'
find .github/workflows -maxdepth 1 -type f -print | sort | xargs wc -lRepository: DASDAE/dascore
Length of output: 5021
Disable persisted checkout credentials where checkout credentials are not needed.
actions/checkout preserves the GitHub token by default; set persist-credentials: false on checkout steps whose later workflow actions do not need authenticated Git. Add it to the changed checkout steps, and verify the .github/actions/mamba-install-dascore jobs before applying it there.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 34-37: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 9 files
.github/workflows/build_deploy_master_docs.yaml#L34-L37(this comment).github/workflows/build_deploy_stable_docs.yaml#L29-L32.github/workflows/get_coverage.yml#L13-L16.github/workflows/lint.yml#L19-L22.github/workflows/profile.yml#L30-L33.github/workflows/run_min_dep_tests.yml#L37-L37.github/workflows/run_min_dep_tests.yml#L61-L64.github/workflows/runtests.yml#L41-L41.github/workflows/runtests.yml#L66-L69.github/workflows/test_doc_build.yml#L18-L21.github/workflows/upload_pypi.yml#L12-L15
🤖 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 @.github/workflows/build_deploy_master_docs.yaml around lines 34 - 37,
Disable persisted checkout credentials by adding persist-credentials: false to
the checkout steps in .github/workflows/build_deploy_master_docs.yaml (34-37),
.github/workflows/build_deploy_stable_docs.yaml (29-32),
.github/workflows/get_coverage.yml (13-16), .github/workflows/lint.yml (19-22),
.github/workflows/profile.yml (30-33), both sites in
.github/workflows/run_min_dep_tests.yml (37 and 61-64), both sites in
.github/workflows/runtests.yml (41 and 66-69),
.github/workflows/test_doc_build.yml (18-21), and
.github/workflows/upload_pypi.yml (12-15). Verify the
.github/actions/mamba-install-dascore jobs first and do not change them unless
their later steps do not require authenticated Git.
Source: Linters/SAST tools
| if self.ndim > 1: | ||
| msg = "Select only works on 1D coords." | ||
| raise CoordError(msg) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require exactly one dimension, not at most one. The new self.ndim > 1 checks admit rank-0 coordinates, breaking the prior 1D-only contract.
dascore/core/coords.py#L375-L377: useself.ndim != 1before sample-array selection.dascore/core/coords.py#L460-L462: useself.ndim != 1before alignment.dascore/core/coords.py#L949-L951: useself.ndim != 1before sample-count calculation.dascore/core/coords.py#L1247-L1249: useself.ndim != 1before resizing a partial coordinate.tests/test_core/test_coords.py#L2581-L2617: addCoordPartial(shape=())regression cases for these APIs.
📍 Affects 2 files
dascore/core/coords.py#L375-L377(this comment)dascore/core/coords.py#L460-L462dascore/core/coords.py#L949-L951dascore/core/coords.py#L1247-L1249tests/test_core/test_coords.py#L2581-L2617
🤖 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/core/coords.py` around lines 375 - 377, Require exactly one dimension
in all affected coordinate APIs by changing the ndim guards in
dascore/core/coords.py ranges 375-377, 460-462, 949-951, and 1247-1249 from
greater-than-one checks to not-equal-one checks. Add regression cases in
tests/test_core/test_coords.py range 2581-2617 using CoordPartial(shape=()) to
verify sample-array selection, alignment, sample-count calculation, and
partial-coordinate resizing reject rank-0 coordinates.
7630a16 to
fdcfde3
Compare
Description
Routine forward-integration of
masterintodev. Brings the 7 commits merged tomastersince the last integration into thedevbranch (#767 coverage, #768 CoordRange fast paths, #766/#755/#753/#747/#745, and the assert→exception conversions in #765/#755).Merge branched from
origin/dev; four files conflicted and were resolved by hand:dascore/io/wav/core.py— kept dev'scoerce_to_local_path/coerce_to_upathresource handling and adopted master'sassert→ParameterErrorconversion for the single-patch guard.dascore/proc/correlate.py— kept dev's removal of thelagparameter (raisesTypeErrorwhenlagis passed via kwargs) and adopted master'sassert→ParameterErrorconversion for the 2D-patch guard. Master's deprecation-warning branch was dropped becauselagis no longer a function parameter on dev.tests/test_core/test_coords.py— kept both new test classes (TestStringCoordsfrom dev,TestDimensionalityErrorsfrom master).tests/test_io/test_wav/test_wav.py— kept both new tests. Master'stest_multi_patch_spool_raiseswas updated: two identical patches coalesce into a single logical patch under dev's spool semantics, so the test now offsets a copy in time to build a genuine 2-patch spool before asserting the write raises.Changelog
none
Checklist
I have (if applicable):
Verification
pre-commit run --files <changed>— passed (ruff, ruff-format, future-annotations, typos, etc.).pytest tests— 8151 passed, 89 skipped, 2 xfailed in ~120s.Summary by CodeRabbit
New Features
gap_color,gap_factor).Bug Fixes
db=True.Performance