fix estimate_nbands function

[matthewkuner]

Fixes the estimate_nbands function. See #3148 for more context.

[janosh]

@matthewkuner Thanks for spotting this!

Would be great if @aaron-kaplan or someone else more qualified than me could sign off on this.

Also, @matthewkuner we need a unit test in pymatgen/io/vasp/tests/test_sets.py for this change in behavior. It should pass with and fail without this change. Maybe in test_nelect.

pymatgen/pymatgen/io/vasp/tests/test_sets.py

Line 233 in 71864db

def test_nelect(self):

[aaron-kaplan]

Would be great if @aaron-kaplan or someone else more qualified than me could sign off on this.

You tagged the wrong person. I'm not involved with this project.

[janosh]

@aaron-kaplan Apologies.

I meant @esoteric-ephemera.

[esoteric-ephemera]

@matthewkuner, lines 694-695 seem wrong to me per our discussion:

nmag = len([m for m in self.incar["MAGMOM"] if not np.allclose(m, 0)])
nmag = np.ceil((nmag+1)/2)

The lines of code in VASP's main.F are:

NMAG=SUM(T_INFO%ATOMOM(1:T_INFO%NIONS))
NMAG = (NMAG+1)/2

which to me looks like VASP is simply summing the initial magnetic moments (ATOMOM), not counting the number of atoms with nonzero magnetic moments

The weird thing is that NMAG is an integer but ATOMOM is declared to be a float in poscar_struc.F. Fortran seems to do int + float = int + floor(float) when assigning to a variable. You probably want to double check your code against a few sets of INCAR's and OUTCAR's.

Also, @matthewkuner, if the user specifies NPAR (if 'NPAR' is in self.incar), you probably want to add a line as in main.F:

NBANDS=((NBANDS+NPAR-1)/NPAR)*NPAR

@janosh, my name is unfortunately too common to get it as a username 😂

[Andrew-S-Rosen]

@matthewkuner: Copying over our brief convo from Slack, it would perhaps be nice (if you're willing to do so) to move the estimate_nbands definition out of the DictSet class. It would be nice to be able to call estimate_nbands for a given Structure without instantiating a DictSet.

[matthewkuner]

@janosh @arosen93 @esoteric-ephemera Please see comments in #3148 by @mkhorton

[janosh]

The license concerns? Perhaps we can loop in someone from VASP and get their ok. Haven't looked into this myself, but taking @matthewkuner's word for it, I doubt VASP minds us fixing downstream repercussions arising from disparities between their docs and their actual implementation. Given it's a paid product, this should not have happened in the 1st place.

[Andrew-S-Rosen]

Perhaps @MichaelWolloch could help clarify or get us in touch with folks who can.

I also see this as being a likely non-issue.

[mkhorton]

Yes, agreed--just asking the VASP devs is the best course of action here. I also agree hopefully it's a non-issue, but good to check.

[matthewkuner]

@janosh I've updated the tests and verified them to quick VASP calcs that I ran.

[janosh]

Also, @matthewkuner, if the user specifies NPAR (if 'NPAR' is in self.incar), you probably want to add a line as in main.F:
NBANDS=((NBANDS+NPAR-1)/NPAR)*NPAR

Thanks @matthewkuner! Did you address this comment raised by @esoteric-ephemera?

[matthewkuner]

@janosh I addressed everything from Aaron’s comment except for the NPAR stuff. I’ll do that tomorrow

[mkhorton]

I’m assuming since work is proceeding that VASP devs ok’d using main.F as a reference? Would it be possible to include a copy of the response here for the record?

[janosh]

No response yet from VASP. Not sure who to ping. @sudarshanv01 Do you have any suggestions?

[matthewkuner]

@janosh @mkhorton I asked a similar question about NGX/Y/Z, and they basically just said that as long as it isn't a direct copy-paste, it should be fine. See: https://www.vasp.at/forum/viewtopic.php?p=24351#p24351

Considering it is being transcribed from one language to another, I think we are fine here. Let me know if you think my interpretation is too biased/generous to our efforts.

[mkhorton]

I think that’s reasonable. I just wanted to make sure there was a record. Perhaps this thread can be linked on that forum post as well just to be completely above board and leave it there if there are no objections.

[janosh]

Thanks @matthewkuner! Great catch. 👍

[MichaelWolloch]

Hi, and apologies for the late reply. I am on holidays this week and did not check my emails for a while. I have never dealt with this type of code base related issue, and am hesitant to comment either way. Maybe @henriquemiranda is in-office and knows more about VASPs policy here? Or can quickly ask someone more senior?
Cheers, M

[shyuep]

This code has no tests whatsoever. Can someone pls add a unittest for it? @janosh Pls make sure that this is always done for any new code before merging.

[matthewkuner]

@shyuep there are indeed new tests for this PR, check the second file changed

[shyuep]

@matthewkuner Ah yes. However, it says skip if no PSP directory. That basically means it is not running at all on Github Actions. Can the test be modified to at change check a few POTCAR files? Since the code is not running on the CI, we can't detect any bugs until someone runs it on their computer.