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

Update bader_caller.py #2117

Merged
merged 5 commits into from
May 3, 2021
Merged

Update bader_caller.py #2117

merged 5 commits into from
May 3, 2021

Conversation

nwinner
Copy link
Contributor

@nwinner nwinner commented Apr 27, 2021

Summary

My last PR for the bader caller had some issues, which this PR resolves. (edited after discussion below)

  • Fix 1: Previously, I had "self.nelects" for cube analysis created by looking for the site property with that name. It didn't occur to me that the self.structure object is not user-defined, but created from the cube file, and this property will never exist. I removed this, and changed the functions to have an optional nelect(s) argument for when parsing cube files.

  • Feature 1: I have added get_decorated_structure method. Which is like the previous "get_spin_decorated_structure", but more general. Assuming you ran bader with an appropriate reference using chgref_filename, this lets you assign a generic property to a structure using the reference file for the partitioning and the main file for the integrating. This can be, for example, be used to parse a spin density cube file to get spin decorated structure or a hartree cube file to get a electrostatic potential decorated structure.

Update to bader_caller. nelect needs to be an argument, as it can actually never be inferred from the structure. Spin decorated structure was also a neat idea, but isn't compatible with bader analysis.
@mkhorton
Copy link
Member

Thanks @nwinner. For point 2, doesn’t the Bader binary already support this? (Ie getting volumes from the charge density, then integrating the spin density using those volumes) I seem to recall this feature was available in the Bader caller to do this automatically.

@nwinner
Copy link
Contributor Author

nwinner commented Apr 27, 2021

I hadn't seen that before, but after googling it looks like you can use

bader CHGCAR_spin -ref CHGCAR_sum.

If you generated ACF using that method, then you would get a spin decorated structure by calling get_charge_decorated_structure(), but you would have to flip the sign...

I'm not sure how to best make that a part of this module though. Thoughts?

@mkhorton
Copy link
Member

mkhorton commented Apr 27, 2021 via email

@coveralls
Copy link

coveralls commented Apr 27, 2021

Coverage Status

Coverage decreased (-0.7%) to 83.007% when pulling 0b7955a on nwinner:patch-1 into df0f9ba on materialsproject:master.

@nwinner
Copy link
Contributor Author

nwinner commented Apr 27, 2021

Okay so I don't think we need a reference field in decorated structure. It does look like just specifying cghcar_ref when initializing BaderAnalysis, will let you parse a spin density file using the electron density as a reference. So it works as is.

The name is not great, as chgcar_ref does seem vasp-specific, but its really a "general" reference as far as bader is concerned.

As for how to change get_decorated_structure, should I put "spin decorated structure" back in, with with the knowledge that you have to set the reference chgcar appropriately, or maybe, should it get_charge_decorated-structure be changed to allow for this? I'm hesitant to rename things because of compatibility.

can be used for spin decorating, electrostatic potential decorating,
etc. so long as a reference file is used with it.
on my machine doesn't... not sure what that is about, but I've manually
changed to match what the github workflow says should be correct.
@nwinner
Copy link
Contributor Author

nwinner commented Apr 28, 2021

Okay the newest commit should has a function called get_decorated_structure(). What do you think @mkhorton ?

@mkhorton
Copy link
Member

mkhorton commented May 3, 2021

Thanks @nwinner, I clarified the docstring a bit otherwise this looks good to me -- ok to merge?

@nwinner
Copy link
Contributor Author

nwinner commented May 3, 2021

Looks okay to me. Hopefully this doesn't need to be updated again anytime soon.

@mkhorton mkhorton merged commit ce2fb4e into materialsproject:master May 3, 2021
@nwinner nwinner deleted the patch-1 branch May 3, 2021 21:56
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.

None yet

3 participants