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

Workflow for Quasi-harmonic approximation (forcefields and VASP) #903

Merged
merged 51 commits into from
Sep 9, 2024

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Jun 28, 2024

Continues #902 .

ToDo:

  • Reduce number of steps in the workflow
  • Finalize Force field tests
  • Check all units and add field descriptions
  • Extend the tests
  • Fix all linting issues
  • add options for different eos fits
  • add a VASP Workflow (with minimal settings)
  • Job names are updated
  • Check execution order (resolved)
  • Add tests for VASP workflow (due to issues with the naming convention, I reduced the VASP test workflow to 1 optimization, 1 eos structures, 1 phonon run and skipped the analysis step)
  • check born_maker initialisation
  • Add large amounts of data to data store
  • Figure out good vasp settings for the optimization and how to deal with prev_dir
  • Document the new workflows

@JaGeo
Copy link
Member Author

JaGeo commented Jun 29, 2024

I tracked it down. As soon as I initialize PhonopyQHA, I cannot jsanitanize the document. Even in cases when none of the outputs are used for the document. This is an extremely weird error and I might have to go through the phonopy source to understand where this might be coming from.

@JaGeo
Copy link
Member Author

JaGeo commented Jun 29, 2024

I think it is this line here: https://github.com/phonopy/phonopy/blob/04f912a77e3efbac00a9987b545fc4f5e8927eb9/phonopy/qha/eos.py#L134

Afterwards all warnings are turned into exceptions and none of the tests are running as before anymore.

@JaGeo
Copy link
Member Author

JaGeo commented Jul 10, 2024

@tpurcell90 this one would be nearly ready. If you have time, you could have a look here.

@tpurcell90
Copy link
Contributor

Yes do you want me to do directly in this PR or in a separate branch that gets merged in (not sure if there are concurrent changes going on)?

@JaGeo
Copy link
Member Author

JaGeo commented Jul 10, 2024

@tpurcell90 i have stopped working on it for now. Tests have been passing and I am waiting for further feedback from @utf .

But you might want to use your own branch starting from this one.

@tpurcell90
Copy link
Contributor

Okay I will start my own branch and then merge things in once done just in case

@tpurcell90
Copy link
Contributor

It might make more sense for #889 to be fully merged in before I adapt this one. Since that one seems to just need a final merge. Since this also uses eos workflows I don't want to conflict with what @ansobolev did

@JaGeo
Copy link
Member Author

JaGeo commented Jul 10, 2024

Ah, I see! @tpurcell90

@JaGeo
Copy link
Member Author

JaGeo commented Jul 10, 2024

@tpurcell90 to add, i don't rely on the full eos workflow as it would be too hard to modify. Therefore, I would not expect many conflicts.

src/atomate2/vasp/flows/qha.py Outdated Show resolved Hide resolved
-------
.PhononMaker
"""
logging.log(level=0, msg="test")
Copy link
Member Author

Choose a reason for hiding this comment

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

remove

Comment on lines +115 to +121
self.eos = CommonEosMaker(
initial_relax_maker=self.initial_relax_maker,
eos_relax_maker=self.eos_relax_maker,
static_maker=None,
postprocessor=None,
number_of_frames=self.number_of_frames,
)
Copy link
Member Author

@JaGeo JaGeo Aug 1, 2024

Choose a reason for hiding this comment

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

@utf I am currently wondering about how to initalize and reuse the EosMaker in the best way. Unfortunately, starting solely from the EosMaker for the qha workflow would take too much time. This is why I only partly reuse and extend it here. Should I do it similarly to the Grüneisen workflow and add the EosMaker as an argument for the overall qhamaker?

Copy link
Member Author

Choose a reason for hiding this comment

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

@utf ? any opinions?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused - what do you mean by:

starting solely from the EosMaker for the qha workflow would take too much time

Either way, I'm open to both approaches:

  • Adding the EosMaker as an argument would reduce the number of options in the CommonQhaMaker class but make it more difficult to set properties such as the number of frames. I don't think this is too much of an issue if we could include an example of how to configure the settings in the docstring/documentation page.
  • The current internal use of CommonEosMaker makes it easier to configure settings but is slightly less clean code. By the way, I noticed the linear_strain option is not currently used.

I'll let you decide!

Copy link
Member Author

@JaGeo JaGeo Aug 23, 2024

Choose a reason for hiding this comment

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

Thanks! I think I will go with 2.

What I meant: instead of relying on phonopy to deal with the equation of state fitting, to reuse the fitting options from the EosStateMaker. (EDIT: there was another comment here. I mixed something up.)

Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

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

Thanks for this @JaGeo, it looks in a very nice state. Just some minor points from me.

Comment on lines +115 to +121
self.eos = CommonEosMaker(
initial_relax_maker=self.initial_relax_maker,
eos_relax_maker=self.eos_relax_maker,
static_maker=None,
postprocessor=None,
number_of_frames=self.number_of_frames,
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused - what do you mean by:

starting solely from the EosMaker for the qha workflow would take too much time

Either way, I'm open to both approaches:

  • Adding the EosMaker as an argument would reduce the number of options in the CommonQhaMaker class but make it more difficult to set properties such as the number of frames. I don't think this is too much of an issue if we could include an example of how to configure the settings in the docstring/documentation page.
  • The current internal use of CommonEosMaker makes it easier to configure settings but is slightly less clean code. By the way, I noticed the linear_strain option is not currently used.

I'll let you decide!

Comment on lines +78 to +87
def prev_calc_dir_argname(self) -> str:
"""Name of argument informing static maker of previous calculation directory.

As this differs between different DFT codes (e.g., VASP, CP2K), it
has been left as a property to be implemented by the inheriting class.

Note: this is only applicable if a relax_maker is specified; i.e., two
calculations are performed for each ordering (relax -> static)
"""
return "prev_dir"
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually needed any more? VASP, CP2K etc all have the same dir name now prev_dir. I guess we coud remove this from any other workflows that have it?

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 think yes because abinit has different names.

Copy link
Member Author

Choose a reason for hiding this comment

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

phonon_jobs = []
outputs = []
for istructure, structure in enumerate(eos_output["relax"]["structure"]):
phonon_job = phonon_maker.make(structure)
Copy link
Member

Choose a reason for hiding this comment

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

If possible, it would be nice to pass the prev_dir from the relaxation? That way we can make use of auto_ispin and make the calculations cheaper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did this now by extending the EOS dictionaries.

@JaGeo
Copy link
Member Author

JaGeo commented Sep 3, 2024

@utf I would be really happy if this could be merged. I would like to show this in an upcoming presentation (~2 weeks).

@JaGeo JaGeo enabled auto-merge (squash) September 9, 2024 20:18
@JaGeo JaGeo merged commit f1894c1 into materialsproject:main Sep 9, 2024
6 checks passed
@utf utf added the feature A new feature being added label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature being added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants