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

Added trace of M*overlap #768

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented May 8, 2024

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.

Copy link

codecov bot commented May 8, 2024

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 ⚠️
Additional details and impacted files
@@            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     

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

@zerothi
Copy link
Owner

zerothi commented May 14, 2024

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
Copy link
Contributor Author

pfebrer commented May 14, 2024

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
Copy link
Contributor Author

pfebrer commented May 14, 2024

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

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

@zerothi
Copy link
Owner

zerothi commented May 14, 2024

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
Copy link
Contributor Author

pfebrer commented May 14, 2024

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
Copy link
Owner

zerothi commented May 21, 2024

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
Copy link
Contributor Author

pfebrer commented May 21, 2024

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.

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