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

Improve Lobster workflow preconverge step, kpoints, docs #277

Merged
merged 17 commits into from
Mar 22, 2023

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Mar 21, 2023

Summary

I have now implemented some of the suggestions from #275 (same number of bands in preconverge static step and static step).

I also changed the number of kpoints. Again, I think some more tests are necessary to confirm that this reciprocal density of kpoints is emough.

I added a tutorial on how to access the lobsterpy plots from the database.

@JaGeo JaGeo changed the title Fix preconverge Improve Lobster workflow preconverge step, kpoints, docs Mar 21, 2023
@JaGeo
Copy link
Member Author

JaGeo commented Mar 21, 2023

@utf, maybe, we could also check if auto_ispin is useful in this workflow as well.

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #277 (33133e5) into main (6c1f611) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head 33133e5 differs from pull request most recent head 6532893. Consider uploading reports for the commit 6532893 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
- Coverage   65.90%   65.78%   -0.13%     
==========================================
  Files          73       73              
  Lines        7048     7049       +1     
  Branches      901      900       -1     
==========================================
- Hits         4645     4637       -8     
- Misses       2137     2149      +12     
+ Partials      266      263       -3     
Impacted Files Coverage Δ
src/atomate2/lobster/schemas.py 92.82% <ø> (ø)
src/atomate2/vasp/flows/lobster.py 96.36% <100.00%> (+1.91%) ⬆️
src/atomate2/vasp/jobs/lobster.py 95.08% <100.00%> (+1.63%) ⬆️

... and 1 file with indirect coverage changes

@JaGeo
Copy link
Member Author

JaGeo commented Mar 21, 2023

Still not happy with the preconverge step. Will try to work on it further.

@utf
Copy link
Member

utf commented Mar 21, 2023

Ok! I have a feeling using the NonSCF uniform maker will do the trick!

@JaGeo
Copy link
Member Author

JaGeo commented Mar 21, 2023

Yes, you are probably right. I will check it out tomorrow!

@JaGeo
Copy link
Member Author

JaGeo commented Mar 22, 2023

  • Test update (LobsterStaticMaker and LobsterUniformMaker)
  • check output schema for uuids and directories
  • check deletion of WAVECARs
  • remove WAVECAR copying from LobsterStaticMaker
  • Update incar_settings

input_set_generator=StaticSetGenerator(
user_kpoints_settings={"reciprocal_density": 100},
user_incar_settings={
"IBRION": 2,
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove IBRION, NSW, LCHARG, and ISIF as these are set by the static maker anyway (and IBRION, ISIF are not important with NSW = 0)

user_kpoints_settings={"reciprocal_density": 400},
user_incar_settings={
"LWAVE": True,
"NSW": 0,
Copy link
Member

Choose a reason for hiding this comment

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

Similarly you can remove NSW and ICHARG here too.

Copy link
Member Author

@JaGeo JaGeo Mar 22, 2023

Choose a reason for hiding this comment

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

Will ICHARG be set to 11 even if I give different incar settings?

EDIT: I will remove ICHARG as well. I somehow got lost with the settings.

@@ -61,14 +61,14 @@ class LobsterStaticMaker(BaseVaspMaker):
name: str = "static_run"
input_set_generator: VaspInputGenerator = field(
default_factory=lambda: StaticSetGenerator(
user_kpoints_settings={"grid_density": 6000},
user_kpoints_settings={"reciprocal_density": 400},
user_incar_settings={
"IBRION": 2,
Copy link
Member

Choose a reason for hiding this comment

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

And IBRION, ISIF, NSW from here.

Copy link
Member Author

@JaGeo JaGeo Mar 22, 2023

Choose a reason for hiding this comment

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

Short question: should I enable auto_ispin and things like this here as well?

update: I think I will just enable this as it will also save computational time.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think you can enable it. The default is for the lobster workflow to do a relaxation anyway, so I think it could be useful.

@@ -151,6 +159,8 @@ def update_user_incar_settings_maker(
Structure object.
prev_vasp_dir : Path or str
Path or string to vasp files.
append_name: str or None
Copy link
Member

Choose a reason for hiding this comment

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

I guess you don't need append_name here since that was for the preconvergence stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right. I need to clean up.

@JaGeo
Copy link
Member Author

JaGeo commented Mar 22, 2023

I still need to pass the dir_names for the LobsterUniformMaker correctly. Deletion of WAVECAR.gz currently does not work fully. This is hopefully the last remaining issue with this new implementation.

Update: It seems to work fine! I just don't have the dir_name, uuid of the first static_maker of the LobsterUniformMaker in the final schema at the moment. I will try to find out how I can access this information as well.

)
)

def make(self, structure: Structure, prev_vasp_dir: str | Path | None = None):
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion with regard to code reuse? I think I simply copied this part and just adapted the part above.

Copy link
Member

Choose a reason for hiding this comment

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

You could just subclass the UniformBandStructureMaker and remove the make function entirely?

Copy link
Member

Choose a reason for hiding this comment

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

In fact, probably even better is not even to subclass and just define the maker as a constant. E.g.

LOBSTER_UNIFORM_MAKER = UniformBandStructureMaker(
    name: str = "uniform lobster structure"
    static_maker: BaseVaspMaker = field(
        default_factory=lambda: StaticMaker(
            input_set_generator=StaticSetGenerator(
                user_kpoints_settings={"reciprocal_density": 100},
                user_incar_settings={
                    "EDIFF": 1e-7,
                    "LAECHG": False,
                    "LVTOT": False,
                    "LREAL": False,
                    "ALGO": "Normal",
                    "LWAVE": False,
                },
            )
        )
    )
    bs_maker: BaseVaspMaker = field(
        default_factory=lambda: NonSCFMaker(
            input_set_generator=NonSCFSetGenerator(
                user_kpoints_settings={"reciprocal_density": 400},
                user_incar_settings={
                    "LWAVE": True,
                    "ISYM": 0,
                },
            )
        )
    )
)

Copy link
Member

Choose a reason for hiding this comment

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

Jimmy has done it this way in the phonon workflow and means we don't get too many makers. The line between adding a new maker vs reusing the old ones is quite ambitious though, so happy either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the constant in this case. I was thinking about subclassing but this would have added another layer of complexity as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

(btw, I also like that we still get the VASP dos in this case. It also allows to compare Lobster and VASP dos if necessary. thus, I will keep the higher NEDOS setting)

@JaGeo
Copy link
Member Author

JaGeo commented Mar 22, 2023

@utf to me it looks like I currently can only access the output from the uniform_job and nothing else in the UniformBandStructureMaker. Shouldn't we add the dir_name and uuid from the static_job before as well?

return Flow(jobs, uniform_job.output, name=self.name)

@JaGeo
Copy link
Member Author

JaGeo commented Mar 22, 2023

Except for this issue, I am happy with the code for now. I very much hope that this version of the workflow is now fine for standard runs and also showcases the advantages of a workflow manager.

@utf
Copy link
Member

utf commented Mar 22, 2023

I think its ok that you only store the dir_name and uuid of the non-scf step. If you really need it you could use the uuid of the non-scf to get the inputs of that job and get the static dir.

If you really want to include them we could add a all_outputs property to the Flow class in jobflow but this could be quite brittle if your flow maker has an job which isn't a vasp job.

@JaGeo
Copy link
Member Author

JaGeo commented Mar 22, 2023

Thanks! Then, I will leave it this way for now :).
I also found another tiny issue in the schema: when there are no cation-anions, one of the lobsterpy runs will fail and of course no text can be generated. It's fixed in this version.

@utf
Copy link
Member

utf commented Mar 22, 2023

Great, thank you for these changes!

@utf utf merged commit 161f685 into materialsproject:main Mar 22, 2023
@utf utf added the enhancement Improvements to existing features label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants