duartegroup / autodE

automated reaction profile generation
https://duartegroup.github.io/autodE/
MIT License
165 stars 51 forks source link

Fix QChem not using max_core #275

Closed t-young31 closed 1 year ago

t-young31 commented 1 year ago

Resolves #273

Hopefully this is correct!


Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #275 (cf8a7cf) into v1.4.0 (6210a14) will decrease coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           v1.4.0     #275      +/-   ##
==========================================
- Coverage   97.24%   97.19%   -0.05%     
==========================================
  Files         197      197              
  Lines       21013    21026      +13     
==========================================
+ Hits        20434    20437       +3     
- Misses        579      589      +10     
Flag Coverage Δ
unittests 97.19% <100.00%> (-0.05%) :arrow_down:

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

Impacted Files Coverage Δ
autode/wrappers/QChem.py 97.49% <100.00%> (+0.01%) :arrow_up:
tests/test_wrappers/test_qchem.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

t-young31 commented 1 year ago

Thanks @shoubhikraj – I've had a look at the manual now too and I'm not really any the wiser. Seems like the default CC_MEMORY is 50% MEM_TOTAL, but is that in addition to MEM_TOTAL?! @asterlingchem any ideas?

asterlingchem commented 1 year ago

From my interpretation of https://manual.q-chem.com/latest/Ch6.S16.SS2.html and https://manual.q-chem.com/latest/Ch6.S16.SS4.html, when using the default CCMAN2 library, CC_MEMORY is set to 50%* of MEM_TOTAL, so you shouldn't need to specify CC_MEMORY explicitly in most cases (assuming the default has been sensibly chosen). It's a similar story for MEM_STATIC – this value is set automatically as 12% of MEM_TOTAL. So as long as MEM_TOTAL is set, and is appropriate for the resources available on the node, then CC_MEMORY and MEM_STATIC shouldn't need to be specified.

*This is a bit weird because the recommendation from 6.16.2 of the manual is to use 75-80% of MEM_TOTAL, but this appears to be for exclusive node use – so maybe the 50% is a safer threshold?

t-young31 commented 1 year ago

awesome – thanks @asterlingchem