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

Ferroelectric Workflow #196

Closed
wants to merge 104 commits into from
Closed

Conversation

fraricci
Copy link
Contributor

@fraricci fraricci commented Nov 2, 2022

This is a first version of the Ferroelectric wf and represents a porting of the the same wf from atomate.

It works properly with a known ferroelectric material (KNbO3), but several things are still missing.

Here a partial list:

  • review the logic
  • add docs
  • add tests
  • review format using pre-commit

It is my first atomate2/jobflow wf therefore any feedback is welcome.
I took inspiration from other wf in core.py and elastic.py.

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #196 (149b012) into main (98f3ed5) will increase coverage by 6.49%.
Report is 926 commits behind head on main.
The diff coverage is 1.82%.

❗ Current head 149b012 differs from pull request most recent head 75ae38d. Consider uploading reports for the commit 75ae38d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
+ Coverage   65.90%   72.40%   +6.49%     
==========================================
  Files          73       55      -18     
  Lines        7048     4870    -2178     
  Branches      901      707     -194     
==========================================
- Hits         4645     3526    -1119     
+ Misses       2137     1158     -979     
+ Partials      266      186      -80     
Files Coverage Δ
src/atomate2/vasp/jobs/core.py 87.50% <75.00%> (-2.09%) ⬇️
src/atomate2/vasp/schemas/ferroelectric.py 0.00% <0.00%> (ø)
src/atomate2/vasp/flows/ferroelectric.py 0.00% <0.00%> (ø)
src/atomate2/vasp/jobs/ferroelectric.py 0.00% <0.00%> (ø)

... and 61 files with indirect coverage changes

@fraricci
Copy link
Contributor Author

Added some initial test mostly.
I tested the wf with Fireworks and with an external output db.

@utf
Copy link
Member

utf commented Mar 2, 2023

Hi @fraricci, thanks for this, it looks great!

Are you able to install and run the pre-commit hook?

pip install pre-commit
pre-commit install
pre-commit run --all-files

This will then get run automatically any time you commit anything to github.

@fraricci
Copy link
Contributor Author

fraricci commented Mar 5, 2023

Sure, I'll give it a try soon.

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.

Hi @fraricci, thanks very much for this. Its looking very nice and will be a great feature to have in atomate2.

I've added some detailed comments that move the workflow more in line with atomate2 vs atomate1.

Happy to answer any questions if things aren't clear.

Thanks again!

src/atomate2/vasp/flows/ferroelectric.py Show resolved Hide resolved
src/atomate2/vasp/flows/ferroelectric.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/ferroelectric.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/ferroelectric.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/ferroelectric.py Outdated Show resolved Hide resolved
outputs = {}

for i, interp_structure in enumerate(interp_structures[1:]):
interpolation = PolarizationMaker().make(interp_structure)
Copy link
Member

Choose a reason for hiding this comment

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

See previous comments:

  1. You should pass prev_vasp_dir
  2. You should pass the polarisation maker into this function to allow customisation.

Can you also add some metadata about the transformations. E.g. a dict of the initial structure, final structure, and interpolation index? As this will allow the reconstruction of the interpolation later on in a builder. E.g., see what we did in the elastic workflow:

elastic_relax_maker.write_additional_data["transformations:json"] = ts

Note, this uses transformations.json but you could write any json file you'd like. E.g., the electron-phonon workflow writes a custom file:

info = {
"temperature": temp,
"original_structure": original_structure,
"supercell_structure": supercell_structure,
}
elph_job.update_maker_kwargs(
{"_set": {"write_additional_data->elph_info:json": info}}, dict_mod=True
)

src/atomate2/vasp/schemas/ferroelectric.py Show resolved Hide resolved
src/atomate2/vasp/schemas/ferroelectric.py Outdated Show resolved Hide resolved
src/atomate2/vasp/schemas/ferroelectric.py Outdated Show resolved Hide resolved
@fraricci
Copy link
Contributor Author

Glad you like it! Thanks a lot for revising the whole thing. I had quick look at your comments and suggestions.
They actually cover most of the questions I had when I was implementing this wf.
I'll get back to this and implement you suggestions soon.
Thanks!

@fraricci fraricci changed the title [WIP] Ferroelectric Workflow Ferroelectric Workflow Oct 7, 2024
@utf
Copy link
Member

utf commented Oct 8, 2024

Hi @fraricci, please can you merge main into this branch? Something seems to have happened to it and now there are 621 files changed.

@fraricci
Copy link
Contributor Author

fraricci commented Oct 8, 2024

Hey! Yeah, I think I messed this branch up trying to sync it to the upstream with git rebase. I'll check what I can do.

@fraricci
Copy link
Contributor Author

fraricci commented Oct 8, 2024

This branch got broken in some way. New and cleaner pull request opened #1012. This one can be closed.

@fraricci fraricci closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.