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

Correct dealiasing in minimal_callsafe_copy #269

Merged
merged 6 commits into from
Apr 22, 2024
Merged

Conversation

pablosanjose
Copy link
Owner

Closes #267

We use the approach described in that issue: redefine minimal_callsafe_copy methods throughout Quantica to restore aliasing relations between subparts of objects, particularly between GreenSolvers and any parent ph::ParametricHamiltonian.

We also fixed a silly bug that called call! on each ph[hybrid(dn)] indexing, so this might even improve performance a bit. I didn't profile any potential allocations regressions of the PR. The changes are subtle and hence a bit risky.

comments

GFUnit type fix

whitespace

minimal_callsafe_copy(s::GreenSlicer) fallback

don't call! when indexing ph[hybrid(...)]

test fixes

test fix

test fix

comments
@pablosanjose pablosanjose changed the title Correct dealiasing issues in minimal_callsafe_copy Correct dealiasing in minimal_callsafe_copy Apr 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 92.41%. Comparing base (6ab78f6) to head (49a538b).

Files Patch % Lines
src/types.jl 82.35% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
- Coverage   92.44%   92.41%   -0.03%     
==========================================
  Files          37       37              
  Lines        6061     6091      +30     
==========================================
+ Hits         5603     5629      +26     
- Misses        458      462       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pablosanjose
Copy link
Owner Author

A twist to this plot:

In a g::GreenFunction with g.solver::SparseLUGreenSolver, the solver need to alias it's parent g.contacts, because it incorporates the contact self energies into the factorization problem (instead of adding it after the fact using a T-matrix approach). Hence, if we create g, and then do g´ = minimal_callsafe_copy(g), the link between the two is broken. To restore it we need to pass not only parentham but also contacts to minimal_callsafe_copy(g.solver, parentham, contacts).

MWE

julia> glead = LP.linear() |> @onsite((; o = 1) -> o) - hopping(1) |> greenfunction(GS.Schur(boundary = 0));

julia> g = LP.linear() |> -hopping(1) |> supercell |> attach(glead) |> greenfunction;

julia>= Quantica.minimal_callsafe_copy(g);

julia> (0, o = 0)[]  # should be a finite number, but contacts don't update anymore
1×1 OrbitalSliceMatrix{Matrix{ComplexF64}}:
 NaN + Inf*im

@pablosanjose
Copy link
Owner Author

pablosanjose commented Apr 22, 2024

So from this the following general rule is extracted that clarifies this whole PR.

The allowed aliasing relations are strictly the following:

  • A AppliedGreenSolver may alias a parent AbstractHamiltonian and a parent Contacts, precisely those in the GreenFunction that contains that solver. Hence, minimal_callsafe_copy(::AppliedGreenSolver, ...) needs both of these as extra arguments in general.
  • An AbstractSelfEnergySolver cannot alias anything outside of itself. Hence minimal_callsafe_copy(::AbstractSelfEnergySolver) is single-argument.

This begs the question: why shouldn't we move g.contacts and g.parent into g.solver for each AppliedGreenSolver? That would simplify minimal_callsafe_copy, which would become single-argument always. Ah, but not really, since this move would not remove the aliasing relations between deeply nested parts of the solver, such as the SchurFactorSolver and the AppliedSchurGreenSolver, so some multi-argumen minimal_callsafe_copy would have to remain.

@pablosanjose pablosanjose merged commit e0421b9 into master Apr 22, 2024
9 checks passed
@pablosanjose pablosanjose deleted the greenclosure2 branch April 24, 2024 16:05
@pablosanjose pablosanjose mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken closure in GreenFunction
2 participants