zerothi / sisl

Electronic structure Python package for post analysis and large scale tight-binding DFT/NEGF calculations
https://zerothi.github.io/sisl
Mozilla Public License 2.0
182 stars 58 forks source link

Added trace of M*overlap #768

Open pfebrer opened 4 months ago

pfebrer commented 4 months ago

Adds a method to retrieve the trace of the matrix multiplication of a matrix with its overlap.

E.g. to get the total number of electrons from the DM or the band structure energy from the EDM (I think?).

If you agree that this is useful I can add some tests before merging.

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 87.26%. Comparing base (8d6da59) to head (65dda4f). Report is 11 commits behind head on main.

Files Patch % Lines
src/sisl/_core/sparse_geometry.py 25.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #768 +/- ## ========================================== + Coverage 86.91% 87.26% +0.35% ========================================== Files 410 393 -17 Lines 51826 50238 -1588 ========================================== - Hits 45042 43841 -1201 + Misses 6784 6397 -387 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zerothi commented 4 months ago

I am thinking whether this would be more appropriate as a regular method:

def trace(self, axis1=0, axis2=1, use_overlap=False)...

then it can be used for more than 1 thing?

Probably we should add the offset argument, but currently only allow offset=0 for simplicity. I guess it could be useful down the road? What do you think?

pfebrer commented 4 months ago

Does np.trace already work?

I don't know if it's a good idea to introduce the matrix multiplication on a simple trace method, because it feels like an arbitrary choice (you could add any other operation). For example, if you use the use_overlap argument, what does use mean here? Would it be clear that it means matrix multiplication?

In terms of generalization I was thinking more about something like:

def matmul_trace(self, other, ...):

So that you could do for example $Tr[H*\rho]$ to get the energy.

pfebrer commented 4 months ago

But it is also true that you can acheive that simply with np.sum(H * dm) :thinking:

I wonder if having the explicit method helps, because I had not thinked that it was that easy until now :sweat_smile:

zerothi commented 4 months ago

Does np.trace already work? Well, I haven't tested it, but I guess it should.

I don't know if it's a good idea to introduce the matrix multiplication on a simple trace method, because it feels like an arbitrary choice (you could add any other operation). For example, if you use the use_overlap argument, what does use mean here? Would it be clear that it means matrix multiplication?

I think it would be clear from a Hamiltonian perspective. But perhaps the wording isn't sufficient. apply_overlap, or ... ?

In terms of generalization I was thinking more about something like:

def matmul_trace(self, other, ...):

So that you could do for example Tr[H∗ρ] to get the energy.

Hmm, I think we shouldn't do complicated functions that are allready standardized, e.g. matmul and trace. I think trace is sufficiently standard in terms of our matrices that a trace of H could work with and without S.

But it is also true that you can acheive that simply with np.sum(H * dm) 🤔 Well, I think the sum would also return the S * S sum which you typically don't want, no?

I wonder if having the explicit method helps, because I had not thinked that it was that easy until now 😅

I think allowing the standard methods would be fine enough to overwrite in some form.

pfebrer commented 4 months ago

Hmm, I think we shouldn't do complicated functions that are allready standardized, e.g. matmul and trace.

Yes, but the point is that trace(matmul(X)) doesn't actually require matmul(X)

Well, I think the sum would also return the S * S sum which you typically don't want, no?

Hmm that is another point. Wouldn't it make sense that operations on sparse orbital matrices didn't modify the overlap (if present)?

zerothi commented 4 months ago

Hmm, I think we shouldn't do complicated functions that are allready standardized, e.g. matmul and trace.

Yes, but the point is that trace(matmul(X)) doesn't actually require matmul(X)

Well, I think the sum would also return the S * S sum which you typically don't want, no?

Hmm that is another point. Wouldn't it make sense that operations on sparse orbital matrices didn't modify the overlap (if present)?

I would assume that operations remove the overlap (if it involves changing stuff, e.g. trace). I don't know exactly how we should go about this, but re-using standard names would be optimal IMHO. np.trace(..., with_overlap=True) would be fine I think, basically the same as your method name, trace_with_S.

We should do something with __array_ufunc__ to make these things work.

pfebrer commented 4 months ago

I think it is simpler to make something like np.sum(H * H.S) work. That doesn't necessarily involve separating the overlap in storage as I understand you propose in #770.