Skip to content

Conversation

@leburgel
Copy link
Member

When profiling some AD-based optimizations, I noticed the twists that occur in the rrules of the tensor operations show up quite prominently sometimes. This was caused by the fact that by default twist copies its input before calling twist!, even for tensors with bosonic braiding for which twist! does nothing in the end.

Here I just added the check for trivial twists from twist! to twist to avoid a copy for trivial twists. I don't know if anything really relies on the fact that twist makes a copy though. If this is the case, maybe there's a different way to handle trivial twists in the tensor operations pullbacks themselves?

@codecov
Copy link

codecov bot commented Oct 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/tensors/indexmanipulations.jl 71.69% <100.00%> (+0.20%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lkdvos
Copy link
Member

lkdvos commented Oct 26, 2025

This feels like a pretty big breaking change and I'm a little scared of this introducing silent and subtle bugs.

In principle, for the other "indexmanipulations" we have a copy = false keyword to indicate if data sharing with the input is allowed, which is probably the easiest solution. Unfortunately, if we then want to make this non-breaking we would have to set that to true by default, which is not in line with the other default values.

The fastest way forward for tousle specific case is probably to just special case it in the reverse rules, which is in line with the forward rules special case.

However, I think it would be nicest in the long run to just additionally include the keyword as a breaking change, and then possibly simplify the use in the contractions, with the next 0.16 release. I don't know what you think @Jutho ?

@Jutho
Copy link
Member

Jutho commented Oct 26, 2025

We are anyway making breaking changes, I assume this one is quite innocent, so if it is bundled in a breaking release and listed as such, I do not see why we could not introduce the copy keyword with false als default value.

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