Skip to content

feat: implement Base.circshift and Base.circshift!#8

Merged
AFeuerpfeil merged 1 commit intomainfrom
fix/broadcast-style
Apr 1, 2026
Merged

feat: implement Base.circshift and Base.circshift!#8
AFeuerpfeil merged 1 commit intomainfrom
fix/broadcast-style

Conversation

@AFeuerpfeil
Copy link
Copy Markdown
Member

Summary

  • Implements circshift(arr, shifts), circshift!(arr, shifts) (in-place), and circshift!(dest, src, shifts) for PeriodicArray
  • The implementation materializes the new unit cell by evaluating src[k - s] for each index k, correctly applying the boundary map (e.g. Bloch phase factors) for elements that wrap across cell boundaries — unlike a naive circshift(parent(arr), shifts) which would ignore the map at the seam
  • Multiple specific method signatures are used to resolve all method ambiguities with Base (verified by Aqua)
  • Fixes a linter-introduced regression where Vararg{Int, N} was widened to Vararg{Integer, N} in getindex/setindex!, which had reintroduced 4 method ambiguities

Test plan

  • circshift(a, s)[i] == a[i - s] for all integers i, verified across many shifts (including wrapping cases) for both identity and non-trivial maps
  • The "seam" element circshift(a, 1)[1] == a[0] == f(data[end], -1) is correctly computed with the map applied
  • Roundtrip: parent(circshift(circshift(a, s), -s)) == parent(a) for all shift values
  • circshift!(dest, src, shifts) (3-arg) and circshift!(arr, shifts) (in-place) both tested
  • All 956 tests pass, Aqua reports 0 method ambiguities

🤖 Generated with Claude Code

Adds circshift (non-mutating), circshift! (in-place 2-arg), and
circshift! (3-arg dest/src) for PeriodicArray.

The implementation materializes the new unit cell by evaluating
src[k - s] for each index k, correctly applying the map (e.g. Bloch
phase factors) for elements that wrap across cell boundaries. This
requires the map to satisfy the group action property
map(map(x, a), b) == map(x, a+b), consistent with the existing
reverse and repeat implementations.

Also fixes a linter-introduced regression where Vararg{Int, N} was
widened to Vararg{Integer, N} in getindex/setindex!, which had
reintroduced method ambiguities detected by Aqua.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/PeriodicArrays.jl 89.28% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@AFeuerpfeil AFeuerpfeil merged commit f20fb65 into main Apr 1, 2026
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant