VASP Defect Code #215
VASP Defect Code #215
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #215 +/- ##
==========================================
+ Coverage 75.46% 75.57% +0.10%
==========================================
Files 55 55
Lines 4716 4872 +156
Branches 685 701 +16
==========================================
+ Hits 3559 3682 +123
- Misses 975 1007 +32
- Partials 182 183 +1
|
lint lint lint lint lint lint lint lint
| @@ -672,7 +672,7 @@ def from_vasp_files( | |||
|
|
|||
| vasprun_kwargs = vasprun_kwargs if vasprun_kwargs else {} | |||
| volumetric_files = [] if volumetric_files is None else volumetric_files | |||
| vasprun = Vasprun(vasprun_file, **vasprun_kwargs) | |||
| vasprun = Vasprun(vasprun_file, parse_potcar_file=True, **vasprun_kwargs) | |||
There was a problem hiding this comment.
So what I said about Structure.charge vs Structure._charge was wrong.
This was the culprit of my HSE caculations running with the wrong charge states.
I think this setting should always be on for vasprun parsing since we are running in a directory that has POTCARs
There was a problem hiding this comment.
^ This was wrong too it ended up being the CONTCAR copy during the Calculation parsing step.
This is fixed as part of this PR.
atomate2/src/atomate2/vasp/schemas/calculation.py
Lines 500 to 504 in b3f6f3e
|
For Posterity, something like the function below is needed for now to perform the postprocessing without periodic regridding def submit_defect(defect, relax_maker, bulk_dir=None, run=False):
"""Submit a defect calculation"""
ss = MPStaticSet(defect.get_supercell_structure())
ng, ngf = ss.calculate_ng()
ng_settings = dict(zip(["NGX", "NGY", "NGZ", "NGXF", "NGYF", "NGZF"], ng + ngf))
relax_maker = update_user_incar_settings(relax_maker, ng_settings)
maker = FormationEnergyMaker(
relax_maker=relax_maker,
)
flow = maker.make(defect=defect, bulk_supercell_dir=bulk_dir)
return flow |
There was a problem hiding this comment.
Hi Jimmy, this is looking fantastic. I really like the abstraction support for other DFT codes.
My main comment is the update_bulk_maker stuff. The way other workflows handle different settings for different calculations is just by having a different maker altogether. I think this is conceptually simpler and easier to track down why certain parameters are being set.
I expanded on this more in one of the comments, but can you see a big advantage to using this? If not, can we stick with the default way of handling this (which would also be supported in the code agnostic case) and just have two makers?
I was also thinking we should stick with ISIF=2 for the bulk relaxation for now, as if you want to use ISIF=3, you also need to set the custom grid stuff so it will be difficult for other users until the regridding stuff is implemented. I also talk about this in another comment. I personally think this should be fine (since other research groups do this). But if you have strong feelings I'm happy to discuss.
Anyway, it looks fantastic and I'm excited to use it.
|
|
||
|
|
||
| @job( | ||
| name="bulk supercell", |
| uc_structure: Structure, | ||
| relax_maker: RelaxMaker, | ||
| sc_mat: NDArray | None = None, | ||
| update_bulk_maker: Callable | None = None, |
There was a problem hiding this comment.
Again, think it is cleaner to remove the update_bulk_maker function since it could be handled more cleanly with just a separate maker to begin with.
| } | ||
| # TODO: check that the charge state was set correctly | ||
|
|
||
| return Response(output=all_chg_outputs, replace=defect_q_jobs) |
There was a problem hiding this comment.
Same with this here. E.g., see
atomate2/src/atomate2/vasp/jobs/elastic.py
Lines 226 to 227 in aa0bb07
There was a problem hiding this comment.
You're right the outputs are all references. It's just that they are all meant to be comsumed by other jobs so this error did not affect anything. I merged your change.
Co-authored-by: Alex Ganose <[email protected]>
Co-authored-by: Alex Ganose <[email protected]>
Co-authored-by: Alex Ganose <[email protected]>
|
Fantastic, thanks Jimmy! |
VASP Defect Code
Once the general defect code is merged from #214 we can look at the VASP-specific implementation of the defect workflow will just instantiate the general one with some VASP-specific defaults.
I removed the final aggregation step since I believe that's best done with an additional database for the competing phases.
So the current workflow is only responsible for spawning the defect calculations.