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

stdoutSileSiesta.read_charge returns dq and e? #699

Closed
zerothi opened this issue Mar 7, 2024 · 15 comments
Closed

stdoutSileSiesta.read_charge returns dq and e? #699

zerothi opened this issue Mar 7, 2024 · 15 comments

Comments

@zerothi
Copy link
Owner

zerothi commented Mar 7, 2024

Describe the issue

stdoutSileSiesta.read_charge returns a dataframe with columns:

  • dq
  • e
    I think this might be a bit misleading.

Either we should have:

  • de, e
  • dq, q
    I don't think siesta has a history of using e, while I can understand its use.

I think for consistency, it would be wise to streamline these? So what should we choose?
@ahkole and @pfebrer I ping you because you have played with it :)
Any other comments would be appreciated!

@pfebrer
Copy link
Contributor

pfebrer commented Mar 8, 2024

I always use the dq column, so not changing it to de would be more convenient to me 😅

However, I'm not sure it makes sense to use q, since it will always be negative.

@zerothi
Copy link
Owner Author

zerothi commented Mar 8, 2024

Well, I think we need to discern this.

Currently, my understanding of the output is actually that dq is de, i.e. it is in units of electrons. Hence, for correctness one should actually return -dq. So we are now left with a double problem ;)

@ahkole
Copy link
Contributor

ahkole commented Mar 8, 2024

Currently, my understanding of the output is actually that dq is de, i.e. it is in units of electrons.

Are you sure about this? The first few lines of the parsed Hirshfeld charges from one of my calculations is,

 	dq              e 	          S 	          Sx 	          Sy 	   Sz
atom 						
0 	0.46978 	13.53022 	2.96865 	2.96865 	-0.0 	-0.00000
1 	0.46978 	13.53022 	2.96865 	2.96865 	-0.0 	-0.00000
2 	-0.15659 	7.15659 	0.01046 	0.01046 	-0.0 	0.00007

From this it seems like dq assigns a negative charge to excess electrons, so then it would actually be -de right?

@zerothi
Copy link
Owner Author

zerothi commented Mar 8, 2024

Yes, for Hirshfeld and Voronoi, these things got cleaned up. But I think for TS and Mulliken, this is not the case. I might be forgetting here.. But it would be nice to not get into problems here :)

@ahkole
Copy link
Contributor

ahkole commented Mar 8, 2024

If we had to decide to choose either dq + q or de + e then I would prefer to talk about occupations rather than charge, such that excess electrons would be positive and deficit of electrons would be negative (so de + e). This does go against @pfebrer 's preferences though. And if we're changing things I would actually prefer to use n and dn to talk about occupations/populations because I feel e could also be confused with the electron charge constant.

@zerothi
Copy link
Owner Author

zerothi commented Mar 8, 2024

What about having both for some time?

@ahkole
Copy link
Contributor

ahkole commented Mar 8, 2024

Would also be fine for me.

@pfebrer
Copy link
Contributor

pfebrer commented Mar 8, 2024

If we had to decide to choose either dq + q or de + e then I would prefer to talk about occupations rather than charge, such that excess electrons would be positive and deficit of electrons would be negative (so de + e). This does go against @pfebrer 's preferences though. And if we're changing things I would actually prefer to use n and dn to talk about occupations/populations because I feel e could also be confused with the electron charge constant.

Isn't it electrons that we are talking about here? Occupancy could be occupancy of anything 😅 I think "e" is clearer than "n".

What about having both for some time?

I think this would be a bad idea unless one of them is lazily computed. But for that we would need to subclass DataFrame. I think it is better to just choose one and stick with it, whichever it is. There are many parts of sisl where the API is not stable yet for good reason, but I think this one is not a good reason to purposely break future compatibility 😅

@ahkole
Copy link
Contributor

ahkole commented Mar 8, 2024

Isn't it electrons that we are talking about here? Occupancy could be occupancy of anything 😅 I think "e" is clearer than "n".

Fair point. Still I feel like using e could be prone to confusion. I think the least confusing single letter header would be q, but then you would always have negative charges of atoms.

@zerothi
Copy link
Owner Author

zerothi commented Mar 8, 2024

fair points all. :)

@zerothi
Copy link
Owner Author

zerothi commented Mar 11, 2024

Ok, so we stick with dq meaning the charge on atoms.
A deficit of electrons is a positive dq.
A surplus of electrons is a negative dq.

Lets stick with that. :)

@pfebrer
Copy link
Contributor

pfebrer commented Mar 11, 2024

And what about e? I mean do we also keep it?

@zerothi
Copy link
Owner Author

zerothi commented Mar 11, 2024

Yes, lets keep it. :)

@pfebrer
Copy link
Contributor

pfebrer commented Mar 11, 2024

Ok, so this issue is the identity operator 😄

@zerothi
Copy link
Owner Author

zerothi commented Mar 11, 2024

ha :) true :)

@zerothi zerothi closed this as completed Mar 11, 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

No branches or pull requests

3 participants