Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FiniteMPS getindex #218

Merged
merged 8 commits into from
Dec 25, 2024

Conversation

VictorVanthilt
Copy link
Contributor

This PR implements getindex for FiniteMPS using both Integers and AbstractUnitRanges.

@VictorVanthilt
Copy link
Contributor Author

@lkdvos can you look at the implementation as well as if the Base.@propagate_inbounds magic would work with this? I haven't worked with this before.

@VictorVanthilt VictorVanthilt enabled auto-merge (squash) December 20, 2024 18:02
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/states/finitemps.jl 79.55% <100.00%> (+1.03%) ⬆️

... and 1 file with indirect coverage changes

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 okay to me!

For the boundscheck business: the long story short (ish) is that @inbounds will bypass code that is marked within a @boundscheck block, on the condition that it is inlined, or passed through a function barrier with @propagate_inbounds.

Boundschecking itself happens by the function checkbounds, which has two signatures: checkbounds(Bool, args...) and checkbounds(args...). The first outputs a bool to check the bounds, the second calls the first and throws if necessary.

In general, the boundschecking really only matters for performance-critical sections, or expensive boundschecks. I like putting an extra check in the function, as this gives clearer error messages, but putting @inbounds should only be used as a last resort, since that can lead to segfaults.

Could you try and add some tests for this, testing inferred, proper error throwing etc? Nothing too major, but just a few lines to ensure everything is (and keeps) functioning property would be great.

src/states/finitemps.jl Outdated Show resolved Hide resolved
src/states/finitemps.jl Outdated Show resolved Hide resolved
src/states/finitemps.jl Outdated Show resolved Hide resolved
@VictorVanthilt VictorVanthilt merged commit d288385 into QuantumKitHub:master Dec 25, 2024
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.

2 participants