Skip to content

Fix rotl90, rotr90 and rot180 for LocalOperator#267

Merged
lkdvos merged 8 commits intomasterfrom
pb/fix-operator-rotation
Oct 2, 2025
Merged

Fix rotl90, rotr90 and rot180 for LocalOperator#267
lkdvos merged 8 commits intomasterfrom
pb/fix-operator-rotation

Conversation

@pbrehmer
Copy link
Collaborator

@pbrehmer pbrehmer commented Oct 1, 2025

I stumbled upon this bug when I was trying to run SU for Hamiltonians with non-uniform physical spaces. It turns out that the rotation functions for LocalOperators would only work for $1\times1$ unit cells or uniform lattices - this should fix it. (I hope my cold-ridden brain actually did the right thing to fix it but I added some tests so it should be fine, hopefully.) I also renamed the functions for index rotations to remove the type piracy.

@codecov
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/algorithms/correlators.jl 100.00% <100.00%> (ø)
src/operators/localoperator.jl 88.70% <100.00%> (+1.09%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Looks good to me, left some minor comments.
If we are changing the name, I would also be happy to do something like: rotate_left_90, rotate_right_90, rotate_180, and use that everywhere, but obviously that's not a requirement.

@pbrehmer
Copy link
Collaborator Author

pbrehmer commented Oct 1, 2025

If we are changing the name, I would also be happy to do something like: rotate_left_90, rotate_right_90, rotate_180, and use that everywhere, but obviously that's not a requirement.

If we really want to change that everywhere, then I'd do that in a separate PR since we use rotl90 and rotr90 a lot throughout the code. In principle I would be open for that but I'm also fine with keeping a separate function for the CartesianIndex rotations. Reasons being:

  • Using rotl90 and rotr90 for all other purposes makes a lot of sense matching the logic from Base and we want to avoid type piracy
  • The CartesianIndex rotations require a different function signature where the unit cell size needs to be supplied so I feel that this is in spirit slightly different than the other rotl90 and rotr90 methods

@pbrehmer pbrehmer requested a review from lkdvos October 1, 2025 16:46
@lkdvos lkdvos merged commit 1a23361 into master Oct 2, 2025
50 of 51 checks passed
@lkdvos lkdvos deleted the pb/fix-operator-rotation branch October 2, 2025 11:25
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.

3 participants