non_scf_job for vasp calculations
#1840
Conversation
|
Can one of the admins verify this patch? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1840 +/- ##
=======================================
Coverage 99.40% 99.40%
=======================================
Files 81 81
Lines 3170 3200 +30
=======================================
+ Hits 3151 3181 +30
Misses 19 19 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@yw-fang: Thank you very much for your contribution! This is off to a great start. I have provided some comments here that will hopefully help streamline this function. Please let me know if you have any questions.
Once that is completed, tests will need to be added. They can be found in quacc/tests/core/recipes/vasp_recipes/mocked/test_vasp_recipes. You can follow the examples in there to make a new test case for the nscf_job, evaluating the parameters to make sure they are correct given a set of inputs. The VASP calculator is "mocked" in this test suite (see conftest.py for details), so it is not running the VASP executable but rather EMT in the background.
Additionally, you will want to add this recipe to the list of recipes in the documentation.
Finally, you may (if you wish) add yourself to the contributors section in the documentation.
src/quacc/recipes/vasp/core.py
Outdated
| def nscf_job( | ||
| atoms: Atoms, | ||
| preset: str | None = "BulkSet", | ||
| copy_files: SourceDirectory | dict[SourceDirectory, Filenames] | None = None, |
There was a problem hiding this comment.
Since the NSCF job always must come after an SCF job, we should make sure the user always is providing details about where the previous directory is.
With this in mind, I would suggest making a required prev_dir: SourceDirectory positional argument that comes right after atoms: Atoms. This would be used in place of copy_files, which can be removed. We should mention in the docstring that, at minimum, the prev_dir must contain the vasprun.xml and CHGCAR files.
src/quacc/recipes/vasp/core.py
Outdated
| atoms: Atoms, | ||
| preset: str | None = "BulkSet", | ||
| copy_files: SourceDirectory | dict[SourceDirectory, Filenames] | None = None, | ||
| kpoints_mode: str = "line", |
There was a problem hiding this comment.
We can use a clearer type hint on kpoints_mode: from typing import Literal and then use Literal["uniform", "line"] instead of the more generic str.
src/quacc/recipes/vasp/core.py
Outdated
| copy_files: SourceDirectory | dict[SourceDirectory, Filenames] | None = None, | ||
| kpoints_mode: str = "line", | ||
| calculate_optics: bool = False, | ||
| bandgap: float = None, |
There was a problem hiding this comment.
The type hint for bandgap should be float | None.
src/quacc/recipes/vasp/core.py
Outdated
| kpoints_mode: str = "line", | ||
| calculate_optics: bool = False, | ||
| bandgap: float = None, | ||
| vasprun: Vasprun = None, |
There was a problem hiding this comment.
We won't need the vasprun keyword argument because the user will provide details about the prior calculation via prev_dir.
src/quacc/recipes/vasp/core.py
Outdated
|
|
||
| updates: dict[str, Any] = {} | ||
|
|
||
| if vasprun is not None: |
There was a problem hiding this comment.
Before this line, I would suggest using the SourceDirectory specified via prev_dir to generate the Vasprun object.
That would look something like:
from monty.os.path import zpath
from pathlib import Path
from pymatgen.io.vasp.outputs import Vasprun
vasprun = Vasprun(zpath(Path(prev_dir, "vasprun.xml")))You can then remove the if vasprun is not None line because there will always be a vasprun.
src/quacc/recipes/vasp/core.py
Outdated
| if vasprun is None: | ||
| return 5001 |
There was a problem hiding this comment.
We can assume a vasprun is provided and remove this logic.
src/quacc/recipes/vasp/core.py
Outdated
|
|
||
| if kpoints_mode == "uniform": | ||
| # Automatic setting of NEDOS using the energy range and the energy step | ||
| n_edos = _get_nedos(vasprun, 0.005) # Example value, adjust as needed |
There was a problem hiding this comment.
We should have the dedos here be an optional keyword argument that the user can supply. Let's also use the 0.02 default (i.e. dedos: float = 0.02) unless you suggest otherwise.
src/quacc/recipes/vasp/core.py
Outdated
| # Use tetrahedron method for DOS and optics calculations | ||
| updates.update({"ismear": -5, "isym": 2, "nedos": n_edos}) | ||
| elif kpoints_mode == "line": | ||
| sigma = 0.2 if bandgap == 0 else 0.01 |
There was a problem hiding this comment.
Since the bandgap produced by VASP may not be exactly 0 yet still be metallic, we should have this be something more like if bandgap < 1e-4 else 0.01 instead.
src/quacc/recipes/vasp/core.py
Outdated
| if calculate_optics: | ||
| # LREAL not supported with LOPTICS = True; automatic NEDOS usually | ||
| # underestimates, so set it explicitly | ||
| n_edos = _get_nedos(vasprun, 0.0025) # Example value, adjust as needed | ||
| updates.update( | ||
| {"loptics": True, "lreal": False, "cshift": 1e-5, "nedos": n_edos} | ||
| ) |
There was a problem hiding this comment.
I'd personally suggest we remove this block for now. If we want to allow optical calculations, we can have a dedicated optics_job that essentially calls the nscf_job with some updated parameters to add. This will help keep the nscf_job easier to read.
src/quacc/recipes/vasp/core.py
Outdated
| calc_defaults=calc_defaults, | ||
| calc_swaps=calc_kwargs, | ||
| additional_fields={"name": "VASP NSCF"}, | ||
| copy_files=copy_files, |
There was a problem hiding this comment.
Since the user is providing a prev_dir, we need to define copy_files here. We don't need to copy every file in the directory, so we can safely do in the line before this function call:
copy_files = {prev_dir: ["CHGCAR*", "WAVECAR*"]}
src/quacc/recipes/vasp/core.py
Outdated
| # Validate kpoints_mode | ||
| supported_modes = ("line", "uniform") | ||
| if kpoints_mode not in supported_modes: | ||
| raise ValueError(f"Supported kpoints modes are: {', '.join(supported_modes)}") |
There was a problem hiding this comment.
You can remove this section and instead have the check be done a few lines below in the section that looks like:
if kpoints_mode == "uniform":
...
elif kpoints_mode == "line":
...
else:
raise ValueError(f"Supported kpoint modes are...")
src/quacc/recipes/vasp/core.py
Outdated
| print( | ||
| "Warning: vasprun.xml* file does not exist in the specified directory." | ||
| ) |
There was a problem hiding this comment.
Logging statements should be used instead of print statements. See here for an example. That said, I don't think we need these at all (see below).
src/quacc/recipes/vasp/core.py
Outdated
| for file_name in basics_to_copy[1:]: # actually it checks the existence of WAVECAR* | ||
| matching_files = [ | ||
| file for file in os.listdir(prev_dir) if file.startswith(file_name[:-1]) | ||
| ] | ||
| if not matching_files: | ||
| print( | ||
| f"Warning: {file_pattern} file does not exist in the specified directory." | ||
| ) |
There was a problem hiding this comment.
We already log a warning if specified files aren't found. We should aim to keep the recipe as clean and minimal as possible. Since this check is already done internally, we should remove this block. I also don't know that we need to have the other FileNotFoundErrors above. The warning will be raised, and VASP will crash. That is enough. We shouldn't try to overengineer things too much.
src/quacc/recipes/vasp/core.py
Outdated
| if vasprun_exists: | ||
| vasprun_path = Path(prev_dir, "vasprun.xml") | ||
| if (vasprun_path_gz := Path(str(vasprun_path) + ".gz")).exists(): | ||
| vasprun_path = zpath( | ||
| vasprun_path_gz | ||
| ) # if vasprun.xml.gz, zpath will decompress it | ||
| vasprun = Vasprun(vasprun_path) | ||
| else: | ||
| print( | ||
| "Warning: vasprun.xml* file does not exist in the specified directory." | ||
| ) |
There was a problem hiding this comment.
As I noted here, this whole block can simply be replaced by
vasprun = Vasprun(zpath(Path(prev_dir, "vasprun.xml")))That's all that's needed. Note that zpath does not decompress anything. It simply adds on the ".gz" extension if it was missing. The Vasprun() class knows to decompress internally.
|
@yw-fang I left some comments as you're working on this. In general, resist the need to overengineer things :) the earlier version of the code was largely good and only needed the changes suggested. |
src/quacc/recipes/vasp/core.py
Outdated
| } | ||
|
|
||
| # check the expected files in prev_dir | ||
| basics_to_copy = ["CHGCAR*", "WAVECAR*", "vasprun.xml*"] |
There was a problem hiding this comment.
We do not need (and should not) copy the vasprun.xml file to the new VASP working directory.
|
Thanks for bringing this up, @yw-fang! I admit that I haven't given it much thought. I suppose non_scf_job is a bit clearer than nscf, but I'll ultimately leave the decision up to you and your best judgment. |
Thanks for the quick response. I can change the names a little bit to be closer to the implementation of recipes of Quantum Espresso. Although I prefer 'nscf' because Quantum Espresso also used it in the source code, I think keeping the same (or at least similar) style should be better if one user wants to use several different codes to compare the calculation results between them. I don't want to bother them to spend additional time changing the names of the recipes, and they can just focus their attention on the specific parameters depending on the codes. Let me know what you think! |
|
I'll trust your judgment on this one. 👍 |
|
The documentation about the recipe and contribution was updated 17314b1 |
Andrew-S-Rosen
changed the title
|
@yw-fang: Thanks for your contribution! I have made some modifications, mainly to add some additional features. Please take a look when you get a moment and let me know what you think. If you're happy with it, then I will merge it in. |
## Summary of Changes Here, I have changed the default NEDOS from 5001 to 3001 in the VASP static jobs. The value of 3001 represents a 10x increase from the default, which seems logical. A dedicated NSCF job will be made with #1840 anyway, which will be more robust in terms of DOS.
Andrew-S-Rosen
changed the title
non_scf_job for vasp calculations
Hello @Andrew-S-Rosen thanks for enhancing this feature. Almost all of them look more professional than I did. Especially I like the One thing I'd like to resume is the |
|
@Andrew-S-Rosen I noticed one of the tests by GitHub actions didn't pass. It seems that the CODECOV_TOKEN expires. |
|
@yw-fang good point. I was thinking about that as well. I will revert the change and have NEDOS be a sufficiently large value. Also, not to worry about codecov. Sometimes, it just misbehaves and we have to try running it again... |
Great! Thanks for your trust! I don't have any further comments now. Quacc and you have motivated me to go back to read some pages about python in the latest week. Thanks! |
|
Merging this. Thank you again! |
Andrew-S-Rosen
merged commit 6e362bf
into
Quantum-Accelerators:main
Summary of Changes
>> It aims to implement the non-SCF job for VASP. Closes #1774 <<
Checklist
main.Notes