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

Remove unnecessary calls to minimal_callsafe_copy #333

Merged
merged 10 commits into from
Feb 6, 2025
Merged

Conversation

pablosanjose
Copy link
Owner

@pablosanjose pablosanjose commented Feb 5, 2025

The calls to minimal_callsafe_copy everywhere are too conservative. The semantics of this function is the following

if x´ = minimal_callsafe_copy(x), then a call!(x, ...; ...) will not affect x´ in any way

Therefore there is no need to do such copies for GreenSolutions and GreenSlicers, since they cannot be call!ed.

This PR, built out of #332 , is an experiment to see if such a cleanup breaks tests

(EDIT) Summary: Quantica.minimal_callsafe_copy must be understood as a partner to Quantica.call!. It need not exist on objects that cannot be changed by a Quantica.call!. But it is convenient to have otherwise for advanced users, even if we don't actually use all its methods in Quantica.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 94.64286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.26%. Comparing base (7ed802c) to head (61c8c34).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/solvers/green/schur.jl 95.55% 4 Missing ⚠️
src/observables.jl 0.00% 1 Missing ⚠️
src/solvers/green/internal.jl 50.00% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
- Coverage   92.48%   92.26%   -0.23%     
==========================================
  Files          41       41              
  Lines        7135     7172      +37     
==========================================
+ Hits         6599     6617      +18     
- Misses        536      555      +19     

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

@pablosanjose
Copy link
Owner Author

In fact, do we ever need to do minimal_callsafe_copy(::GreenFunction)?
If we remove this, we also remove a cascade of minimal_callsafe_copys of solvers, selfenergies, contacts, etc...

The basic guarantees that we want from minimal_callsafe_copy is that if x´ = minimal_callsafe_copy(x), we are allowed to call!(x, ...; ...) without fear of affecting x´ in any way. They need not be completely deasliased, but all fields that may be affected by call! will be dealiased.

The need for minimal_callsafe_copy(h::AbstractHamiltonian) is clearer. If we want to evaluate many Bloch matrices in parallel with minimal allocations using call!(h, ϕ) (in threads, for example) we need a bunch of minimal_callsafe_copies of h.

It is clear from the above that any type that does not support call!, and does not have fields that support call!, and is not affected by call! when it is itself a field of a type that supports call! has it as a field... then it doesn't need to implement minimal_callsafe_copy. This includes Lattice for example. OpenHamiltonian is a field of GS.AppliedSchurGreenSolver, which may be a field of GreenFunction, which can be call!-ed... so it may need to support minimal_callsafe_copy.

This doesn't mean minimal_callsafe_copy is implicitly called by the user with the standard interface. It is an optional partner of Quantica.call!. As such it may not be needed for e.g. GreenFunction, but it's good to have it if we have Quantical.call!.

@pablosanjose pablosanjose changed the title Experimental: remove unnecessary calls to minimal_callsafe_copy Remove unnecessary calls to minimal_callsafe_copy Feb 5, 2025
@pablosanjose pablosanjose merged commit 0a1837b into master Feb 6, 2025
9 checks passed
@pablosanjose pablosanjose mentioned this pull request Feb 6, 2025
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