Skip to content

Remove redefinition of tensorexpr#268

Merged
leburgel merged 6 commits intoQuantumKitHub:masterfrom
Yue-Zhengyuan:remove_tensorexpr
Oct 23, 2025
Merged

Remove redefinition of tensorexpr#268
leburgel merged 6 commits intoQuantumKitHub:masterfrom
Yue-Zhengyuan:remove_tensorexpr

Conversation

@Yue-Zhengyuan
Copy link
Member

@Yue-Zhengyuan Yue-Zhengyuan commented Oct 2, 2025

EDIT: This PR requires MPSKit v0.13.7 or higher.


This PR removes the following redefinition of MPSKit.tensorexpr:

# currently need this because MPSKit restricts tensor names to symbols
_totuple(t) = t isa Tuple ? t : tuple(t)
MPSKit.tensorexpr(ex::Expr, inds::Tuple) = Expr(:ref, ex, _totuple(inds)...)
function MPSKit.tensorexpr(ex::Expr, indout, indin)
return Expr(
:typed_vcat, ex, Expr(:row, _totuple(indout)...), Expr(:row, _totuple(indin)...)
)
end

When I look at the current MPSKit v0.13.6 (actually, since v0.12.6 or QuantumKitHub/MPSKit.jl#256), MPSKit no longer "restricts tensor names to symbols":

https://github.com/QuantumKitHub/MPSKit.jl/blob/440acdb19fb12b7b9e45b2b02ad865ab8adb66a1/src/utility/utility.jl#L123-L128

tensorexpr(name, inds) = Expr(:ref, name, _totuple(inds)...)
function tensorexpr(name, indout, indin)
    return Expr(
        :typed_vcat, name, Expr(:row, _totuple(indout)...), Expr(:row, _totuple(indin)...)
    )
end

Since now PEPSKit depends on MPSKit v0.13 or higher, we should be able to remove the redefinition of tensorexpr safely.

@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/algorithms/contractions/localoperator.jl 95.29% <ø> (+0.31%) ⬆️
🚀 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 2, 2025

Seems like there are still some corner cases left that were not caught by the MPSKit implementation. Do you want to add them to a PR there or just leave this as is for now? In principle this is not a big copy, and tensorexpr is kind of not marked as public in MPSKit anyways

@Yue-Zhengyuan
Copy link
Member Author

All the errors seem to come from reduced_densitymatrix, complaining length(::Symbol) is not defined. It may still be worthwhile understanding what happened...

@lkdvos
Copy link
Member

lkdvos commented Oct 2, 2025

The problem is that tensorexpr(:name, :i, :j) is not caught as one of the edge cases, as the MPSKit version expects tensorexpr(:name, (:i,), (:j,)) instead, and while trying to canonicalize everything, it tries to convert the Symbol into a tuple, calling Tuple(::Symbol) however doesn't do the trick, which is where the error is coming from. You can see that I solved this in PEPSKit with _totuple which does a better job at canonicalizing all of the inputs. There are thus three obvious solutions:

  1. Adapt the MPSKit implementation to use something similar to _totuple so that this can be caught
  2. Adapt the generated functions to call tensorexpr(:name, (:i,), (:j,)) directly instead.
  3. Just leave everything as is 🙃

@Yue-Zhengyuan
Copy link
Member Author

Yue-Zhengyuan commented Oct 2, 2025

There actually is a _totuple defined in MPSKit, which differs from the one in PEPSKit only by (the case of) one letter 😅:

  • PEPSKit: _totuple(t) = t isa Tuple ? t : tuple(t)
  • MPSKit: _totuple(t) = t isa Tuple ? t : Tuple(t)

Then I think we should modify MPSKit.

@Yue-Zhengyuan
Copy link
Member Author

There are some weird segmentation faults in the tests which are also encountered in #273, so they are not caused by the PR, but some upstream regressions.

@leburgel
Copy link
Member

It seems like it could be an issue with Julia v1.12.0 itself. I can't see much difference in package versions at first sight, the differences from the last time CI passed is that Julia v1.12.0 is used versus v1.11.7 before, and along with that LinearAlgebra.jl is on v1.12.0 versus v1.11.7 before. Most likely Julia v1.12.0 broke Zygote?

@lkdvos
Copy link
Member

lkdvos commented Oct 10, 2025

@kshyatt is this what you meant with the thing caused by GC pressure?

@Yue-Zhengyuan
Copy link
Member Author

Might be related to JuliaLang/julia#59138.

@Yue-Zhengyuan Yue-Zhengyuan enabled auto-merge (squash) October 22, 2025 14:41
@leburgel leburgel disabled auto-merge October 23, 2025 07:01
@leburgel leburgel merged commit 64d09ff into QuantumKitHub:master Oct 23, 2025
48 of 51 checks passed
@Yue-Zhengyuan Yue-Zhengyuan deleted the remove_tensorexpr branch October 23, 2025 07:10
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