atomate2 icon indicating copy to clipboard operation
atomate2 copied to clipboard

[WIP] add draft for gruneisen workflow

Open naik-aakash opened this issue 1 year ago • 6 comments

Summary

This PR aims to add a Gruneisen workflow using phonopy API

Todo

  • [ ] Add job to compute Gruneisen parameter to the flow
  • [ ] Add tests
  • [ ] Add a VASP-compatible flow as well (currently only forcefield flow is added)

naik-aakash avatar Feb 27 '24 14:02 naik-aakash

Hi @JaGeo, @joabustamante and @kaueltzen, just tagging you all here for updates.

naik-aakash avatar Feb 27 '24 14:02 naik-aakash

Codecov Report

Attention: Patch coverage is 89.00524% with 21 lines in your changes missing coverage. Please review.

Project coverage is 74.97%. Comparing base (29a5731) to head (580312e). Report is 40 commits behind head on main.

:exclamation: Current head 580312e differs from pull request most recent head e8a77ba

Please upload reports for the commit e8a77ba to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
+ Coverage   74.94%   74.97%   +0.03%     
==========================================
  Files         136      144       +8     
  Lines       10513    11018     +505     
  Branches     1643     1694      +51     
==========================================
+ Hits         7879     8261     +382     
- Misses       2143     2252     +109     
- Partials      491      505      +14     
Files Coverage Δ
src/atomate2/common/schemas/phonons.py 93.63% <100.00%> (-0.45%) :arrow_down:
src/atomate2/common/schemas/gruneisen.py 96.29% <96.29%> (ø)
src/atomate2/forcefields/flows/gruneisen.py 82.60% <82.60%> (ø)
src/atomate2/common/jobs/gruneisen.py 88.67% <88.67%> (ø)
src/atomate2/common/flows/gruneisen.py 83.33% <83.33%> (ø)

... and 3 files with indirect coverage changes

codecov[bot] avatar Feb 27 '24 14:02 codecov[bot]

Thanks!

Can we add that we might need to adapt the current optimization in the force fields, as we don't change the cell shape currently and that we need symmetry and potentially checks that the optimization has converged (especially for force fields?)

JaGeo avatar Feb 27 '24 14:02 JaGeo

If you also want to make this more of a common workflow I can add aims support

tpurcell90 avatar Feb 28 '24 16:02 tpurcell90

@tpurcell90 Surely. It will take some time though. The current Implementation is the result of a first tiny hackathon that we had to introduce jobflow concepts more widely in the group

JaGeo avatar Feb 28 '24 16:02 JaGeo

Ya no rush on that, just ping me when you want me to take a look at it

tpurcell90 avatar Feb 28 '24 16:02 tpurcell90

Hi @JaGeo, We have updated the job to the flow that also includes computing Gruneisen parameters from the phonon runs.

Next, we will add a schema for the flow if desired and also tests

joabustamante avatar Jun 24 '24 16:06 joabustamante

Thank you both, @joabustamante and @naik-aakash !

I left one inital comment. Might need to be checked in the schema itself.

JaGeo avatar Jun 24 '24 17:06 JaGeo

@naik-aakash @joabustamante Great work already! :)

JaGeo avatar Jun 26 '24 18:06 JaGeo

Hi @tpurcell90, We have now added a draft for common flow as BaseGruneisenMaker. It would be great if you could have a look at it and provide us feedback on it, mainly whether it would be fine to use it with FHI-aims in its current state. And also if you have suggestions for the output schema. We have added very minimal schema as of now.

The custom bandstructure plotter in the schema will be moved to Pymatgen in the next few days (it's residing here now just for testing purposes)

We plan to add a VASP-compatible flow soon, but it could take some time.

naik-aakash avatar Jun 28 '24 07:06 naik-aakash

Hi @tpurcell90, We have now added a draft for common flow as BaseGruneisenMaker. It would be great if you could have a look at it and provide us feedback on it, mainly whether it would be fine to use it with FHI-aims in its current state. And also if you have suggestions for the output schema. We have added very minimal schema as of now.

The custom bandstructure plotter in the schema will be moved to Pymatgen in the next few days (it's residing here now just for testing purposes)

We plan to add a VASP-compatible flow soon, but it could take some time.

Ya I can do this, I will try to get this done next week (just flew into Berlin for a visit and have meetings all day today)

tpurcell90 avatar Jun 28 '24 08:06 tpurcell90

I will now try to implement the VASP support.

JaGeo avatar Jul 10 '24 13:07 JaGeo

Hi @tpurcell90, We have now added a draft for common flow as BaseGruneisenMaker. It would be great if you could have a look at it and provide us feedback on it, mainly whether it would be fine to use it with FHI-aims in its current state. And also if you have suggestions for the output schema. We have added very minimal schema as of now. The custom bandstructure plotter in the schema will be moved to Pymatgen in the next few days (it's residing here now just for testing purposes) We plan to add a VASP-compatible flow soon, but it could take some time.

Ya I can do this, I will try to get this done next week (just flew into Berlin for a visit and have meetings all day today)

There is nothing obvious that would prevent FHI-aims from also having this, but I will try to implement it now

tpurcell90 avatar Jul 10 '24 14:07 tpurcell90

@tpurcell90 I started working on the vasp integration. I have already done some small modifications. Maybe, let me push that first. We need different names for the jobs before we can implement any tests

JaGeo avatar Jul 10 '24 14:07 JaGeo

@tpurcell90 I started working on the vasp integration. I have already done some small modifications. Maybe, let me push that first. We need different names for the jobs before we can implement any tests

Okay I will wait for you to be done first and then port it for FHI-aims

tpurcell90 avatar Jul 10 '24 14:07 tpurcell90

@utf should be ready for review. I have now also added tests for the VASP parts.

JaGeo avatar Jul 23 '24 11:07 JaGeo

@jmmshn Just to notify you: the job names of your workflows have slightly changed due to an update on jobflow. I hope this does not affect any of your analysis scripts.

JaGeo avatar Jul 23 '24 17:07 JaGeo

Tests are finally passing.

JaGeo avatar Jul 23 '24 19:07 JaGeo

@jmmshn Just to notify you: the job names of your workflows have slightly changed due to an update on jobflow. I hope this does not affect any of your analysis scripts.

Can you point me to the jobflow PR for this?

jmmshn avatar Jul 24 '24 17:07 jmmshn

@jmmshn Just to notify you: the job names of your workflows have slightly changed due to an update on jobflow. I hope this does not affect any of your analysis scripts.

Can you point me to the jobflow PR for this?

Yes, sure. I am also responsible for this one as it was a requirement for testing any more complex workflows. https://github.com/materialsproject/jobflow/pull/644

I am happy to help to fix the naming for both of your workflows but it is likely faster if you point me to the parts where a name might be appended to a response object in the response object.

JaGeo avatar Jul 24 '24 18:07 JaGeo

@utf I think I implemented all suggestions. Let me know what you think.

JaGeo avatar Jul 27 '24 09:07 JaGeo

@utf thank you for the comments. I will correct the broken sentences in the comments and then wait for your opinion on the settings

JaGeo avatar Jul 30 '24 15:07 JaGeo

@utf should hopefully now be ready to be merged. I had to adapt the code after the new input sets got merged.

JaGeo avatar Jul 31 '24 05:07 JaGeo

Hi @JaGeo , It would be better to now replace the method used for generating Gruneisen bandstrucuture plot in the schema to use the one we implemented in pymatgen, before merging this PR. I will push the change if it is fine with you in sometime today 😃

naik-aakash avatar Jul 31 '24 08:07 naik-aakash

Hi @JaGeo , It would be better to now replace the method used for generating Gruneisen bandstrucuture plot in the schema to use the one we implemented in pymatgen, before merging this PR. I will push the change if it is fine with you in sometime today 😃

Nevermind, seems there are other workflows breaking on updating pymatgen version. So I leave it as is for now and change it later.

naik-aakash avatar Jul 31 '24 09:07 naik-aakash

okay, @naik-aakash !

@utf this would be ready to merge!

JaGeo avatar Jul 31 '24 09:07 JaGeo

This looks great. Thanks @JaGeo @naik-aakash and all other contributors.

utf avatar Jul 31 '24 10:07 utf