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

Move elastic workflow to common and build force-field elastic workflow #581

Merged
merged 33 commits into from
Oct 21, 2023

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Oct 20, 2023

Summary

I have moved the elastic workflow to common using similar design strategies as discussed in #562

Tests are running through.

In the next step, I will work on a elastic workflow for the force fields and tests. I think this shouldn't be too much work but might need some testing with regard to plausible elastic constants ;).

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #581 (1e10ef6) into main (0f63732) will decrease coverage by 0.08%.
The diff coverage is 81.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #581      +/-   ##
==========================================
- Coverage   75.49%   75.41%   -0.08%     
==========================================
  Files          80       83       +3     
  Lines        6728     6793      +65     
  Branches      992     1001       +9     
==========================================
+ Hits         5079     5123      +44     
- Misses       1343     1360      +17     
- Partials      306      310       +4     
Files Coverage Δ
src/atomate2/common/schemas/elastic.py 88.65% <100.00%> (ø)
src/atomate2/forcefields/schemas.py 92.00% <100.00%> (+0.33%) ⬆️
src/atomate2/vasp/flows/amset.py 0.00% <ø> (ø)
src/atomate2/vasp/flows/elastic.py 91.66% <100.00%> (+14.92%) ⬆️
src/atomate2/vasp/jobs/elastic.py 84.61% <ø> (+2.79%) ⬆️
src/atomate2/forcefields/flows/elastic.py 81.81% <81.81%> (ø)
src/atomate2/common/flows/elastic.py 77.55% <77.55%> (ø)
src/atomate2/common/jobs/elastic.py 79.72% <79.72%> (ø)

... and 1 file with indirect coverage changes

@tpurcell90 tpurcell90 mentioned this pull request Oct 20, 2023
5 tasks
@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

I have a first version of the elasticworkflow for forcefields as well. I had to adapt the forcefield schema (voigt vs matrix output).

Is there any benchmark of CHGNet for elastic tensors already that I could use to compare the outputs to? (@janosh ?) At the moment, the output looks very off to me. (Could be still a bug related to the units in here).

@janosh
Copy link
Member

janosh commented Oct 20, 2023

Is there any benchmark of CHGNet for elastic tensors already that I could use to compare the outputs to? (@janosh ?) At the moment, the output looks very off to me. (Could be still a bug related to the units in here).

Sorry to say there's neither an elasticity benchmark yet nor would I be very surprised if the predictions are off by a lot.

There's some good news though. Following the fix in CederGroupHub/chgnet#79, @BowenD-UCB has trained a CHGNet that no longer exhibits the bulk modulus issue described in https://matsci.org/t/51421. The new checkpoint will be part of the upcoming chgnet 0.2.3 release and maybe give better results here as well.

@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

Is there any benchmark of CHGNet for elastic tensors already that I could use to compare the outputs to? (@janosh ?) At the moment, the output looks very off to me. (Could be still a bug related to the units in here).

Sorry to say there's neither an elasticity benchmark yet nor would I be very surprised if the predictions are off by a lot.

There's some good news though. Following the fix in CederGroupHub/chgnet#79, @BowenD-UCB has trained a CHGNet that no longer exhibits the bulk modulus issue described in https://matsci.org/t/51421. The new checkpoint will be part of the upcoming chgnet 0.2.3 release and maybe give better results here as well.

Thanks! I just currently get a bulk modulus of k_vrh=1326.28 GPa for Si. Looks very off to me. I will try with Aluminiumm now. At least, I have something to compare to.

@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

@utf tests seem to pass. The only thing that I am still worried about are the results for CHGNet that are very off (potentially, however, it's just CHGNet itself.)
I would be happy if you could already take a look. I could try running the workflow for a different example or a better potential for stresses.

@JaGeo JaGeo changed the title Move elastic workflow to common and potentially build force-field elastic workflow Move elastic workflow to common and potentially force-field elastic workflow Oct 20, 2023
@JaGeo JaGeo changed the title Move elastic workflow to common and potentially force-field elastic workflow Move elastic workflow to common and build force-field elastic workflow Oct 20, 2023
@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

With a good GAP potential from an old paper, I still don't get very good results. I fear there might be still something wrong.

@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

Found the error. Stresses are simply not in the correct unit (kbar). They were in (eV/Angstrom^3)*10 😅.

With the GAP potential from https://journals.aps.org/prx/pdf/10.1103/PhysRevX.8.041048, I computed a bulk modulus of around 88 GPa. In the paper, they have computed one of ~88.6 GPa (DFT, with GAP they say they have 0÷ rel error). I think this is fine now.

@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

@janosh Could you check the tests, please? I think they have started to randomly fail since you merged your lastest changes.

@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

@utf ready to be reviewed whenever you have time.

@janosh
Copy link
Member

janosh commented Oct 20, 2023

@JaGeo Sorry, some forcefield phonon tests have too tight atol set on assert_allclose. Will fix in #586.

@JaGeo
Copy link
Member Author

JaGeo commented Oct 20, 2023

@JaGeo Sorry, some forcefield phonon tests have too tight atol set on assert_allclose. Will fix in #586.

Thanks! 😃

@JaGeo
Copy link
Member Author

JaGeo commented Oct 21, 2023

Tests are hopefully finally all passing. The forcefields don't produce very consistent results ...

@utf
Copy link
Member

utf commented Oct 21, 2023

Thanks @JaGeo! This is a great feature to have.

@utf utf added the enhancement Improvements to existing features label Oct 21, 2023
@utf utf merged commit 104244d into materialsproject:main Oct 21, 2023
@JaGeo
Copy link
Member Author

JaGeo commented Oct 21, 2023

Thank you, @utf !

@matthewkuner
Copy link
Collaborator

matthewkuner commented Oct 23, 2023

The forcefields don't produce very consistent results ...

@JaGeo how inconsistent are we talking?

@JaGeo
Copy link
Member Author

JaGeo commented Oct 23, 2023

I cannot use an absolute tol of 1e-3. Switched to 1e-1 for the bulk moduli. I haven't tested systematically, however. Depending on the Python version, the results were always a bit different.

I hope this partially answers your question, @matthewkuner

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.

4 participants