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

[Breaking] Fix valence electron configuration parsing for PotcarSingle.electron_configuration #4278

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Feb 1, 2025

Summary

@DanielYang59 DanielYang59 changed the title Fix valence electron configuration determine in PotcarSingle.electron_configuration Fix valence electron configuration determination in PotcarSingle.electron_configuration Feb 1, 2025
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Feb 1, 2025

I have a feeling that we might need to rewrite the electron_configuration property to parse the "Atomic configuration" section of POTCAR instead of generating from Madelung energy ordering rule, as electron configuration from POTCAR may or may not agree with periodic table (now that it's a property of PotcarSingle instead of Element).

For example for the PAW_PBE Fe 06Sep2000 POTCAR, the valence electron configuration is "3d7 4s1" instead of "3d6 4s2":

assert self.psingle_Fe.electron_configuration == [(3, "d", 6), (4, "s", 2)]

   Atomic configuration
    9 entries
     n  l   j            E        occ.
     1  0  0.50        xxx   2.0000
     2  0  0.50        xxx   2.0000
     2  1  1.50        xxx   6.0000
     3  0  0.50        xxx   2.0000
     3  1  1.50        xxx   6.0000
     3  2  2.50        xxx   7.0000
     4  0  0.50        xxx   1.0000
     4  1  1.50        xxx   0.0000
     4  3  2.50        xxx   0.0000

Can you please give me some advice on this? @rkingsbury Thanks in advance

@JaGeo
Copy link
Member

JaGeo commented Feb 1, 2025

@DanielYang59
I agree.
Maybe, take a look at #4262
It partially parses this info already (the code is poorly written but ...)

I also defines a cutoff for an occupied orbital which might help for non-integer cases

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Feb 1, 2025

Hi @JaGeo good day and thanks for the comment

Maybe, take a look at #4262 It partially parses this info already

Beautiful! Looks like the dev_scripts.get_lobster_basis_from_potcars.get_valence function could replace electron_configuration property? In this case do you want me to take over the valence electron property reader part of PotcarSingle (perhaps copy and refactor your get_valence code) and then you work on top of this for the LOBSTER part? Or do you want to work on both?

the code is poorly written but

I guess no code is perfectly written without repeated refactor? :)

10. Constant refactoring is the hallmark of an open platform.

@JaGeo
Copy link
Member

JaGeo commented Feb 1, 2025

@DanielYang59 yes, it would be great if you take over and make this code a method for the potcar. Then, I can adapt the developer code.

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Feb 1, 2025

it would be great if you take over and make this code a method for the potcar

Great! I guess we could either:

  • Simply change (fix) the implement of electron_configuration property (this PR would be breaking then)
  • Or go the safer way, deprecate current electron_configuration with a corrected property (something like val_elec_config or something, naming could be discussed later of course)

Which do you suggest? I might personally prefer the former now that its already giving the wrong results so I would consider this a fix?


I also defines a cutoff for an occupied orbital which might help for non-integer cases

I'm not really aware of non-integer occupancies from POTCARs, can you perhaps point me to some such POTCARs such that I could put more thinking on them?

@JaGeo
Copy link
Member

JaGeo commented Feb 1, 2025

@DanielYang59 i also think it should be the former as it currently is implemented wrong in my opinion.

Li, Be, maybe? Some have 0.01 occupancy, I think. If you have some potcars available, you can use the dev script to spot them.

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Feb 1, 2025

i also think it should be the former as it currently is implemented wrong in my opinion.

Cool I would just replace for now but should be easy to switch anytime

Li, Be, maybe? Some have 0.01 occupancy, I think. If you have some potcars available, you can use the dev script to spot them.

Thanks I would have a look, I thought it's H.* (H.25, H.35, ...)

@DanielYang59 DanielYang59 changed the title Fix valence electron configuration determination in PotcarSingle.electron_configuration [Breaking] Fix valence electron configuration determination in PotcarSingle.electron_configuration Feb 1, 2025
total_electrons = 0.0

for n, subshell, occ in reversed(all_config):
if occ >= 0.01: # TODO: hard-coded occupancy cutoff
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO tag: hard-coded cutoff

@DanielYang59 DanielYang59 changed the title [Breaking] Fix valence electron configuration determination in PotcarSingle.electron_configuration [Breaking] Fix valence electron configuration parsing for PotcarSingle.electron_configuration Feb 1, 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.

Possibly incorrect electron_configuration returned by vasp.inputs.PotcarSingle
2 participants