CompPhysVienna / n2p2

n2p2 - A Neural Network Potential Package
https://compphysvienna.github.io/n2p2/
GNU General Public License v3.0
217 stars 82 forks source link

Feat/add stress #103

Open DanielMarchand opened 3 years ago

DanielMarchand commented 3 years ago

Some points for discussion:

codecov-io commented 3 years ago

Codecov Report

Merging #103 (d7af344) into master (3f03449) will decrease coverage by 0.09%. The diff coverage is 48.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   71.85%   71.75%   -0.10%     
==========================================
  Files         129      129              
  Lines       14022    14049      +27     
==========================================
+ Hits        10075    10081       +6     
- Misses       3947     3968      +21     
Flag Coverage Δ
cpp 74.52% <48.71%> (-0.12%) :arrow_down:
python 71.75% <48.71%> (-0.10%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/libnnp/Structure.cpp 75.66% <48.71%> (-2.87%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3f03449...d7af344. Read the comment docs.

singraber commented 3 years ago

Hey Daniel,

thanks for working on this, looking good so far!

I have one question: maybe I am wrong but I don't think we can have another normalization for the pressure tensor because there is currently already a "normalized" energy and length unit. Since the pressure tensor will be of (energy/length^3) units this already fixes the numeric magnitude, right? I have not thought about this in detail...

However, it is important to consider that many programs will output the pressure tensor in units like kBar or Pa, hence this should be converted before used as an input in the data file. Have you already done this in your example? Thanks a lot anyway for providing an example, this is very helpful!

Best,

Andi

DanielMarchand commented 3 years ago

Hi Andi,

I have one question: maybe I am wrong but I don't think we can have another normalization for the pressure tensor because there is currently already a "normalized" energy and length unit. Since the pressure tensor will be of (energy/length^3) units this already fixes the numeric magnitude, right? I have not thought about this in detail...

This is a very good point and I hadn't thought of it. If this is what happens for the forces then the stresses should follow the same pattern.

However, it is important to consider that many programs will output the pressure tensor in units like kBar or Pa, hence this should be converted before used as an input in the data file. Have you already done this in your example? Thanks a lot anyway for providing an example, this is very helpful!

Thanks for thinking of this, yep, this has already been handled! (In my case the output was in GPa and I've converted everything to Hartree/Bohr^3 units). I actually have the stresses for 5000 structures and a reasonably well-trained NNP for them. A good early goal could be to test the analytic prediction of stresses (we should be able to get results that are at least reasonable for most structures).

Daniel