Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions .agents/skills/draft-release/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,19 @@ Draft the next release version and changelog from merged PRs.

## Workflow

0. Ask for elevated permissions with network access to run `git fetch --all --tags` and the GitHub CLI PR commands used to collect merged PRs (for example `gh pr list`, `gh pr view`, or `gh api`).
1. Fetch the latest refs and tags:
0. Ask for elevated permissions with network access to run the `git fetch` and GitHub CLI PR commands used to collect merged PRs (for example `gh pr list`, `gh pr view`, or `gh api`).
1. Fetch the latest refs and tags from `origin` only. Do **not** use
`git fetch --all` — it also fetches unrelated remotes (e.g. a personal fork or
another contributor's remote) and can fail on those or clobber local tags,
aborting the whole fetch:

```bash
git fetch --all --tags
git fetch --tags origin
```

If you only need to read the tags without touching local state, use
`git ls-remote --tags origin` instead.

2. Determine the next version number:
- Consider only tags that start with `v` and match strict semver:
`^v[0-9]+\.[0-9]+\.[0-9]+$` (ignore pre-release/build suffixes).
Expand All @@ -45,18 +51,32 @@ git fetch --all --tags
- `Bug Fixes`
- `Breaking Changes`

5. Classify PRs deterministically:
- `Breaking Changes` if any of:
- title contains `!` in conventional-commit style segment, or
- label indicates breaking change (e.g., `breaking`), or
- body contains `BREAKING CHANGE`.
- Otherwise `New Features` if labels/titles indicate feature work
(e.g., `feature`, `enhancement`, `feat`).
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.
Comment on lines +54 to +59

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.


6. Classify the remaining PRs. This repo does not use conventional-commit
markers, and its labels are topical (`proc`, `viz`, `spool`, `IO`,
`transform`, `bug`, ...) rather than semantic, so labels alone are not
enough — read each PR's title and body and use judgment:
- `Breaking Changes` if the change removes or alters existing public API,
defaults, or behavior in a way that can break callers — regardless of whether
any `!`, `breaking` label, or `BREAKING CHANGE` text is present. A signature
or keyword change to a documented `Patch`/`dc` method is breaking even when
unlabeled; when unsure, list it here with a one-line note on what changed.
- Otherwise `New Features` if the PR adds a capability, option, or notable
performance improvement (judge from the title/body, not just a
`feature`/`enhancement` label, which is often missing).
- Otherwise `Bug Fixes`.
- Sort entries by PR number ascending.
- Prefer user-facing behavior over internal implementation when deciding and
when summarizing.
- Sort entries within each section by PR number ascending.
- Include a link to the PR in the changelog.

6. Print to screen:
7. Print to screen:
- The new version tag.
- The drafted changelog.

Expand All @@ -73,6 +93,8 @@ Next Version: vX.Y.Z

## Breaking Changes
- #125: Short summary (https://github.com/OWNER/REPO/pull/125)

Reverted (no net change): #126 reverted by #127
Comment on lines +96 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

```

## Notes
Expand All @@ -81,3 +103,4 @@ Next Version: vX.Y.Z
- Prefer explicit, user-facing PR summaries over internal implementation details.
- If no merged PRs are found in scope, still print the next version and include
all sections with `- None`.
- Omit the `Reverted (no net change)` line when no reverted pairs exist.
6 changes: 3 additions & 3 deletions .github/actions/mamba-install-dascore/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ runs:
shell: bash -l {0}
run: echo "CURRENT_DATE=$(date '+%Y-%m-%d')" >> $GITHUB_ENV

- uses: mamba-org/setup-micromamba@v2
- uses: mamba-org/setup-micromamba@v3
with:
micromamba-version: '2.0.5-0' # versions: https://github.com/mamba-org/micromamba-releases
environment-file: ${{ inputs.environment-file }}
Expand Down Expand Up @@ -85,7 +85,7 @@ runs:
- name: restore test data cache
if: "${{ inputs.prepare-test-data == 'true' }}"
id: restore-test-data
uses: actions/cache/restore@v4
uses: actions/cache/restore@v5
with:
path: ${{ env.DATA_CACHE_PATH }}
key: ${{ env.DATA_CACHE_KEY }}
Expand All @@ -97,7 +97,7 @@ runs:

- name: save test data cache
if: "${{ inputs.prepare-test-data == 'true' && steps.restore-test-data.outputs.cache-hit != 'true' }}"
uses: actions/cache/save@v4
uses: actions/cache/save@v5
with:
path: ${{ env.DATA_CACHE_PATH }}
key: ${{ env.DATA_CACHE_KEY }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/build_deploy_master_docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ jobs:
name: github-pages

steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Comment on lines +34 to 37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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 || true

Repository: 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 || true

Repository: 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:


🏁 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 -l

Repository: 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

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/build_deploy_stable_docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
build:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/get_coverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand All @@ -29,7 +29,7 @@ jobs:
run: |
pytest -s --cov dascore --cov-report=xml

- uses: codecov/codecov-action@v4
- uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: ./coverage.xml
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
if: github.event_name == 'push' || !contains(github.event.pull_request.labels.*.name, 'no_ci')

steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/profile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
contains(github.event.pull_request.labels.*.name, 'benchmark')
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/run_min_dep_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
python-matrix: ${{ steps.load-vars.outputs.python-min-deps-matrix }}
os-matrix: ${{ steps.load-vars.outputs.test-os-matrix }}
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
- uses: ./.github/actions/load-shared-vars
id: load-vars

Expand All @@ -58,7 +58,7 @@ jobs:
PYTEST_ADDOPTS: ${{ github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'debug') && '-vv --durations=100' || '' }}

steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/runtests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ jobs:
python-matrix: ${{ steps.load-vars.outputs.python-test-matrix }}
os-matrix: ${{ steps.load-vars.outputs.test-os-matrix }}
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
- uses: ./.github/actions/load-shared-vars
id: load-vars

Expand All @@ -63,7 +63,7 @@ jobs:
PYTEST_ADDOPTS: ${{ github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'debug') && '-vv --durations=100' || '' }}

steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: 'true'
fetch-depth: '0'
Expand Down Expand Up @@ -109,7 +109,7 @@ jobs:
limit-access-to-actor: true

# Upload coverage files
- uses: codecov/codecov-action@v4
- uses: codecov/codecov-action@v5
with:
fail_ci_if_error: false
files: ./coverage.xml
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test_doc_build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
|| (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation'))
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/upload_pypi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
upload:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v5
with:
fetch-tags: "true"
fetch-depth: '0'
Expand Down
28 changes: 28 additions & 0 deletions benchmarks/test_patch_benchmarks.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,16 @@ def test_select(self, example_patch):
patch.select(time=(t1, None))
patch.select(time=(t1, t2))

@pytest.mark.benchmark
def test_update_existing_coords(self, example_patch):
"""Time update_coords re-passing already-validated BaseCoords."""
patch = example_patch
coords = patch.coords
patch.update_coords(
time=coords.coord_map["time"],
distance=coords.coord_map["distance"],
)

@pytest.mark.benchmark
def test_sobel_filter(self, example_patch):
"""Time the Sobel filter."""
Expand All @@ -113,6 +123,18 @@ def test_transpose(self, example_patch):
dims = patch.dims[::-1]
patch.transpose(*dims)

@pytest.mark.benchmark
def test_transpose_noop(self, example_patch):
"""Time a no-op transpose (same dimension order)."""
patch = example_patch
patch.transpose(*patch.dims)

@pytest.mark.benchmark
def test_squeeze_noop(self, example_patch):
"""Time a no-op squeeze (no length-1 dimensions)."""
patch = example_patch
patch.squeeze()

@pytest.mark.benchmark
def test_roll(self, example_patch):
"""Time roll/shift operations."""
Expand All @@ -125,6 +147,12 @@ def test_snap_coords(self, patch_uneven_time):
patch = patch_uneven_time
patch.snap_coords("time")

@pytest.mark.benchmark
def test_snap_coords_already_even(self, example_patch):
"""Time snapping an already even/sorted patch (no-op fast path)."""
patch = example_patch
patch.snap_coords("time", "distance")

@pytest.mark.benchmark
def test_hampel_filter_non_approximate(self, example_patch):
"""Time the Hampel filter."""
Expand Down
38 changes: 34 additions & 4 deletions dascore/core/coordmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@
from typing_extensions import Self

from dascore.constants import dascore_styles, select_values_description
from dascore.core.coords import BaseCoord, CoordSummary, get_coord
from dascore.core.coords import (
BaseCoord,
CoordRange,
CoordSummary,
get_coord,
)
from dascore.exceptions import (
CoordDataError,
CoordError,
Expand Down Expand Up @@ -372,6 +377,9 @@ def _sort_related(name, dim, indexer, new_coords):
cmap = self.coord_map
assert set(coords).issubset(set(cmap))
coords2sort = {x: cmap[x] for x in coords if not getattr(cmap[x], attr)}
# Nothing to sort; return self to avoid rebuilding an identical manager.
if not coords2sort:
return self, array
new_coords, indexers = _get_dimensional_sorts(coords2sort)
if array is not None:
for index in indexers:
Expand Down Expand Up @@ -401,10 +409,17 @@ def snap(
coords = self.dims if len(coords) == 0 else coords
cm, array = self.sort(*coords, array=array, reverse=reverse)
# now the arrays are sorted it should be correct to snap dimensions.
cmap = dict(cm.coord_map)
# Only collect coords whose snap actually changes them so an already
# even manager is returned unchanged (snap returns self when even).
updates = {}
for coord_name in coords:
cmap[coord_name] = cmap[coord_name].snap()
out = cm.new(coord_map=cmap)
current = cm.coord_map[coord_name]
snapped = current.snap()
if snapped is not current:
updates[coord_name] = snapped
if not updates:
return cm, array
out = cm.new(coord_map={**cm.coord_map, **updates})
assert out.shape == self.shape
return out, array

Expand Down Expand Up @@ -851,6 +866,9 @@ def _get_transpose_dims(new, old):
return tuple(new_list)

dims = _get_transpose_dims(new=dims or self.dims[::-1], old=self.dims)
# No-op transpose; return self rather than rebuilding an equal manager.
if dims == self.dims:
return self
return self.new(dims=dims)

def rename_coord(self, **kwargs) -> Self:
Expand Down Expand Up @@ -919,6 +937,9 @@ def squeeze(self, dim: Sequence[str] | None = None) -> Self:
msg = f"cant squeeze dim {name} because it has non-zero length"
raise CoordError(msg)
to_drop.append(name)
# Nothing to squeeze; return self rather than rebuilding an equal manager.
if not to_drop:
return self
return self.drop_coords(*to_drop)[0]

def decimate(self, **kwargs) -> tuple[Self, tuple[slice, ...]]:
Expand Down Expand Up @@ -1186,6 +1207,15 @@ def _get_coord_dim_map(coords, dims):

def _get_coord(coord):
"""Get a coordinate from various inputs."""
# A CoordRange is already canonical (it is the evenly-sampled
# representation), so re-parsing it via model_dump -> get_coord is pure
# overhead; return it directly. Other coord types are NOT short-circuited:
# array coords (CoordArray/CoordMonotonicArray) can be left non-canonical
# by slicing (e.g. an evenly spaced subset that should collapse to a
# CoordRange), and a fully-specified CoordPartial should canonicalize to a
# CoordRange -- get_coord performs that inference.
if isinstance(coord, CoordRange):
return coord
if hasattr(coord, "model_dump"):
coord = coord.model_dump(exclude_defaults=True)
if isinstance(coord, Mapping): # input is a dict
Expand Down
Loading
Loading