From bd574933f18bfc85813d81719e9d261f77662764 Mon Sep 17 00:00:00 2001 From: Tom Young <39765193+t-young31@users.noreply.github.com> Date: Sun, 22 Jan 2023 16:17:23 +0000 Subject: [PATCH 1/7] Ignore doc paths for code CI (#235) * Ignore doc paths for code CI * Update PR template --- .github/pull_request_template.md | 1 + .github/workflows/catch.yml | 3 +++ .github/workflows/pytest.yml | 3 +++ .github/workflows/pytest_cov.yml | 3 +++ 4 files changed, 10 insertions(+) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 2db4fc79e..616b22092 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -11,3 +11,4 @@ Please replace _#ISSUE_ with just e.g. #12 if this PR resolves issue 12. * [ ] The changes include an associated explanation of how/why * [ ] Test pass * [ ] Documentation has been updated +* [ ] Changelog has been updated diff --git a/.github/workflows/catch.yml b/.github/workflows/catch.yml index 537fb17c9..a1e036020 100644 --- a/.github/workflows/catch.yml +++ b/.github/workflows/catch.yml @@ -5,6 +5,9 @@ on: branches: - master pull_request: + paths-ignore: + - 'doc/**' + - 'examples/**' env: BUILD_TYPE: Release diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 8a2e72d9e..83404f85e 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -5,6 +5,9 @@ on: branches: - master pull_request: + paths-ignore: + - 'doc/**' + - 'examples/**' concurrency: group: ${{ github.workflow }}-${{ github.ref }} diff --git a/.github/workflows/pytest_cov.yml b/.github/workflows/pytest_cov.yml index 93d1d9c5e..b797805f5 100644 --- a/.github/workflows/pytest_cov.yml +++ b/.github/workflows/pytest_cov.yml @@ -5,6 +5,9 @@ on: branches: - master pull_request: + paths-ignore: + - 'doc/**' + - 'examples/**' concurrency: group: ${{ github.workflow }}-${{ github.ref }} From be4c9a83e557a365b1973ad4bf173dc85f95b08e Mon Sep 17 00:00:00 2001 From: Shoubhik Maiti <17470159+shoubhikraj@users.noreply.github.com> Date: Mon, 23 Jan 2023 11:19:20 +0000 Subject: [PATCH 2/7] Add "ERROR" level to logging (#231) * add ERROR level to logging * update docs for logging * update tests for logging * update changelog --- autode/log/log.py | 5 ++++- doc/changelog.rst | 19 +++++++++++++++++++ doc/config.rst | 2 +- doc/reference/log.rst | 2 +- tests/test_log.py | 3 +++ 5 files changed, 28 insertions(+), 3 deletions(-) diff --git a/autode/log/log.py b/autode/log/log.py index fb9085001..0bbac83b1 100644 --- a/autode/log/log.py +++ b/autode/log/log.py @@ -3,7 +3,7 @@ """ Set up logging with the standard python logging module. Set the log level with -$AUTODE_LOG_LEVEL = {'', INFO, WARNING, DEBUG} +$AUTODE_LOG_LEVEL = {'', ERROR, WARNING, INFO, DEBUG} i.e. export AUTODE_LOG_LEVEL=DEBUG @@ -36,6 +36,9 @@ def get_log_level(): if log_level_str == "INFO": return logging.INFO + if log_level_str == "ERROR": + return logging.ERROR + return logging.CRITICAL diff --git a/doc/changelog.rst b/doc/changelog.rst index 43ab9f257..ab9c69ad7 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -1,6 +1,25 @@ Changelog ========= +1.3.5 +-------- +---------- + + +Usability improvements/Changes +****************************** +* + + +Functionality improvements +************************** +- + + +Bug Fixes +********* +- Fixes :code:`ERROR` logging level being ignored from environment variable :code:`AUTODE_LOG_LEVEL` + 1.3.4 -------- ---------- diff --git a/doc/config.rst b/doc/config.rst index a3ea6034c..51c0dd8a7 100644 --- a/doc/config.rst +++ b/doc/config.rst @@ -136,7 +136,7 @@ to see all the options. Logging ------- -To set the logging level to one of {INFO, WARNING, ERROR} set the :code:`AUTODE_LOG_LEVEL` +To set the logging level to one of {DEBUG, INFO, WARNING, ERROR} set the :code:`AUTODE_LOG_LEVEL` environment variable, in bash:: $ export AUTODE_LOG_LEVEL=INFO diff --git a/doc/reference/log.rst b/doc/reference/log.rst index 78ac73d51..aa3847dde 100644 --- a/doc/reference/log.rst +++ b/doc/reference/log.rst @@ -11,7 +11,7 @@ Logging Logging ------- -To set the logging level to one of {INFO, WARNING, ERROR} set the AUTODE_LOG_LEVEL +To set the logging level to one of {DEBUG, INFO, WARNING, ERROR} set the AUTODE_LOG_LEVEL environment variable, in bash:: $ export AUTODE_LOG_LEVEL=INFO diff --git a/tests/test_log.py b/tests/test_log.py index e33bedc38..e9bcbe5cd 100644 --- a/tests/test_log.py +++ b/tests/test_log.py @@ -14,6 +14,9 @@ def test_log_level(): os.environ["AUTODE_LOG_LEVEL"] = "DEBUG" assert log.get_log_level() == log.logging.DEBUG + os.environ["AUTODE_LOG_LEVEL"] = "ERROR" + assert log.get_log_level() == log.logging.ERROR + os.environ["AUTODE_LOG_LEVEL"] = "WARNING" assert log.get_log_level() == log.logging.WARNING From c37322ca5aafee0297c7aed3238d1dbad996f564 Mon Sep 17 00:00:00 2001 From: Tom Young <39765193+t-young31@users.noreply.github.com> Date: Mon, 23 Jan 2023 20:58:06 +0000 Subject: [PATCH 3/7] Fix parsing Qchem v6 output (#234) * Fix parsing Qchem v6 output --- autode/__init__.py | 2 +- autode/wrappers/QChem.py | 16 +++++++++----- setup.py | 2 +- tests/test_wrappers/data/qchem.zip | Bin 187427 -> 195544 bytes tests/test_wrappers/test_qchem.py | 33 +++++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 7 deletions(-) diff --git a/autode/__init__.py b/autode/__init__.py index 5789c257e..dbfae35e5 100644 --- a/autode/__init__.py +++ b/autode/__init__.py @@ -39,7 +39,7 @@ - Merge when tests pass """ -__version__ = "1.3.4" +__version__ = "1.3.5" __all__ = [ diff --git a/autode/wrappers/QChem.py b/autode/wrappers/QChem.py index b35c8b2ca..fc1007535 100644 --- a/autode/wrappers/QChem.py +++ b/autode/wrappers/QChem.py @@ -158,17 +158,24 @@ def coordinates_from(self, calc: "CalculationExecutor") -> Coordinates: "Non-optimisation calculation performed - no change" " to geometry" ) - return calc.molecule.atoms + return calc.molecule.coordinates + + if calc.molecule.n_atoms == 1: + # Coordinate of a single atom will not change + return calc.molecule.coordinates coords = [] for i, line in enumerate(calc.output.file_lines): - if "Coordinates (Angstroms)" not in line: + if "Coordinates (Angstroms)" in line: + start_idx = i + 2 + elif "Standard Nuclear Orientation (Angstroms)" in line: + start_idx = i + 3 + else: continue - """e.g. - + """e.g. Coordinates (Angstroms) ATOM X Y Z 1 O 0.0003489977 -0.1403224128 0.0000000000 @@ -177,7 +184,6 @@ def coordinates_from(self, calc: "CalculationExecutor") -> Coordinates: Point Group: cs Number of degrees of freedom: 3 """ - start_idx = i + 2 end_idx = start_idx + calc.molecule.n_atoms coords = [] for cline in calc.output.file_lines[start_idx:end_idx]: diff --git a/setup.py b/setup.py index ddfe8d1d0..530991059 100644 --- a/setup.py +++ b/setup.py @@ -24,7 +24,7 @@ setup( name="autode", - version="1.3.4", + version="1.3.5", python_requires=">3.7", packages=[ "autode", diff --git a/tests/test_wrappers/data/qchem.zip b/tests/test_wrappers/data/qchem.zip index f802e21c216c5132e6505aee5f3a300b32a72444..74565f08223184ddfe31a0029342f21bb7205f33 100644 GIT binary patch delta 9057 zcma)iby$>Z_bv?SNXpPD2qGaJ(hZVBBi)_SITAxR$ROQ zZ~w0Aobz7u&dmGV_ljq&XXc-KFVb%^Is*D{vI;H6e~Iv)MhPdW^l`;Z&&}q>SxpcJ{X(iPKDBwRIC<9M zSg`ORAb{o2(BlDFj(R_XEg>KzPzeww;3IOPxBayJbyq7R((?cH^j~iY1p=vm;Man+ zqW^>$VfM;^hJ<~{<{!?~J3P|AIn^hrd5C{wfvM^Bsp*LBN`K=&6P|({A0aOP&8eR} zMtsDJKKY-F>bK|sCzyZGMNWVr*+0!0>wl(@^Cw0)Zw1EY4CsNt-*amG)sUZWy8Z|Vga}j!P9~OSb}TaN1`duc27f(pzjSbQ zQCGu6Kxh2>U(3Nh9mQNqv#z$yU)9F_v*J_b87r(<+ zeY)7Gk)YV$m-TB$cZl{({lFZrHZz27YWAEr+xEtDmy!o z8FoK3kjuIC;WOa7ZDkQ*o0Y^Pi;uZ&RN%RCC9S__pAwESlcm8{Ry&m_D2Vh+rAo78 z-@6<)(lKUlRbJnwU3l4>ERf%|GHzN=RyTN|g&`m**)t`dOI9}uLHm?BrXgxGYJ?_` zh$m=E`JU-DIPB1x6)#4rCJSlAFAA(0>_h0#T^qSK{TXzaJH#ni<6sO6M8aK`H7c~t z7450ybf(}v43!p-rq)jj67UBTz1Rw< zu}lQF5?ZEWdJNMbJ3iXjA!}SQ`-gmLeu+OOi(UL=WT~DU6 z@YK=A@ktc%6)D7q%8gevo%kk^!7;&XBLklscXo#T2-Yzn{VlXn(+<)36-x!$g-kHU znsEi!u*N2n6&RBJmT`h*R_NK{Mn8ROfo>*KNSak+uO|iyh@GNejyj2QA_*jp7}hp5 zDSALHWTZ0?dk`s}`|SCrg+WKtMP0nEV!Mp+$|*I!4x?!H_ck~nTYgmau@=scz-*1n zF}Lf(h1fiZ@}_(hd37TBM1OYE)_N`H;Ii^Mc<7K-3wPt34nJ3i?3-O}g1lLt{ZW~F zRjhRUZa{T{nahEcT5v=IyIevA|BT86{$ zhD@#66Rs`usbIEvASv!wq+iu5l#XRhdt+ zUF;<9hjW}>ws~tBTRxv_rt_8x;Tp_Y6*0-|)ipJs#UFkl)h7D!DnGkbl-oF2ZyV^> zj*`yU|3ZD}j=lpopW|idxlmKPekIatS?97GWrLksVC6p5P!=yMvjl3h060(wUy~=O z62nvyB0a|#2R6x#MFXaIL+6sBF9??Gm1Sx?vXx8pk%62Zf zR2l1H>XyGNeq~cb&IknG>henxCE-yx!)d@l>+jJWB9FAOdo(B3k{i4_Ir!S`32c&{ za4kw?L>q{|V>=JLn8g4XY4|-k^U+*`TvKorei&DtQo50#mBLhNtb09IWUo{vzpa*6 zL`Lk3tfbiCxCv$8JS{rV5vy3VX2;#JZ9;FAur$H^G>6ApE7O;)HxZUmx`aAA3pF(p zgL`Qz0i8k{XO{I2?WC#}xv+}F1djEK!osH&-qJ>4DK7?vWXDOfwx$AU;5bDPa6p*a z2u^{O%9|1LgAbD8-EP*#ull{T5l#zuk}4mW)d17wX5<<;zaP=g8!|n&Hf$Y<(|2Qm zry^=~m+z~aV5VhmS^rc8!@x1?cnF2U8-lKA1)}U zxVcVOg)sTRH~q1zYYEg4T|pWLRt`GK=~01+oOJ#$8k2`>+o7W{JCa4NxuK2hsJHD2 zG#v^-^SN&|f}ZU83J`Vq>xWlV_`(-5ly7lKHk|?v(Lbbc*4gVVCQCr+wAa|j&EaZ9 zT;w+R^~QI~IzQ~`KUO@88!=a`r4TKRdOy!&{tRBsIY2u$x4NPQQFv>x)SM$Ru7UtF z^!0Y)#%dHyQ%XL^yBpW~zG64gs{7c>t0xnMd2WH2rZ=;LgL%4{@8fmJlw%gErp7bH z8uA9J?25Wvk6JAbF9hxq?3->6McKDi&!!9F)#Bls z`CYp^dG3*qC;QPdcUkCZa-wU^_DPmQUPVV*%*`Nf_R8}|UGSzJByK6*B6gys_$P)~ zD)i+XWJkIzYh~=Y$2%2Yq$;Yr6I}W{^d46%BHQZsoc$Jibt-i5S}NYs0j4gGMN*Z4I3+oLdDl!;6AGvKI6Wf@jW#)^PI{Is zv?b55c8zVjH24f(pEeE03J<_dBgYm?>=HU>{NqXm86Y1ue1sQp*n+2M90nho-|$I5 zXRBV^;KDAoPLHo4F+nvuZ{-%doH8O@UATzBhL+31ZqBwFj7&Sci8AHuq%+TVrd)NR zQ_fvZ@X~VnAGiwxV#j+kOf{cw#INq$iHq>W@&)qoJYCQV)lF2SWy+|zdzM<2Ky-aCB4Q0vDm%}fcWD|~t1Gz|&Qi9G!9*bk|qVTIfdLpou-LD`!2ezQG%u- zPx)=Qw00|$;c*n^2IIK)!pzh!ND86lh?q*3z>2ccPp9yl$w&wGBg?m&;jdv2^l48=1$BlE!U94OfQHw~mh@6UY#!@?xg_N#A zAddW~P{$lOp&^$ECe#W5J>_r&%L&=At|l zhXH@X!R*}e)xezo(Y%f^{I=HEb@ZG)1+<5Q%Rj z%?!dacr@v|l}2CbG}iNuLjH6ehKXmmc4tMZeF@@@nZ+L5~Lc;~`@M@i}*Y~%mgg&3o3rXw_sJ;Tygu$8Ugy=jH$dFSUzxSJ z?e~0X#pFb}lDf2@6Kjnq8yU|TxsZ{l{^q+2KiV95drAF4mdyV>F<)UiQ6(BE$?h9Y znK+y|>XXLq%gT=hGDmD8(%eQ`%VW5Y%o2g~ut<+^Dp92?dAiXri}(+Z5rvUANdQ+Y zc7?Tv0;2?)8r*7}^2ZrL%25!N$|}H_#QJdVz1M&kYRvhfnmV#qUZ$W&z1O!Izo?K~ zL0GEHW1RRU;d&m})rQ{t%%2||%`LPBU3(UfPlcrmM6WC=@4Qa3A}nC7NN<)4BObh4 z1?pEEUo?A3#th8NQ^`$l)6yp9>FTJDT5!DR%w{D$7ZeAKsPqo`GfEe zkomV%x!Fhwp7X+s?NB}c z8=N%QfY;M+wjoU9LpA!VOKcpscZ@rICq|QR&x>4OjEoDsC?P+o8(ml}_HL!?i91(j8`GYW@ z&s8tup$9`w7ak_$74w8?h;=eFVA-<|)e3CS-1azo*)L1(Wzf2QEF|w^+qlVdc7+5# zf=x1mie6WOt~?RL{;$jLt&6?!QBec<@m$#C7Z(=c8uIS=dXqf(kD9X=UwL&%kl``n zVPH;U9*L?g&m<5&8eg3H)a`#&C|ATQV)@Z_)zOizz2KNI;CF4Mlb5pwI7qi>r9RmznC|#R49&Eq3zAp zki)Uv&eNrk$QMB$NP`o;kakz#v!zc-dMYV4u=4lGNh)n^qlFyH1WUUOuj8lBrbkx10F72xy&;KuOpA zrNIs^B!9Ujy)N9+Pw3)9I*CRu!BpgHVSaju=tGRs0XNii`@E29xmWk1CGWC zwUxHvXEBeu>%55s^C$!jSkb3{C7XV!NHi)77EwiiYg53uZTFNiv|sEBfAn-$FK}SM zOIX<`_+8J=Bc0nW$rPU+M%Dbas`hzz=z}MX_Vg{N@w?`$?BwSTWfSfcs&h@W5MB0) zf!M-}bA^4{`@(>hM5He7*eTb*;DrXBRIFFDT5bU+@52VdTbH#?j>su1htGy%(rfWY zjRkiG&v5IKF)y0$fpM{>R{1ZgEL-JWt0cL%dprBlJc2kR9Rp3;0!ub3=eulTJM z+Qe8GiSY$vCT9y&aQ>m!WvZpUc$Se-x`w{WD4A;9T{Umce%S$KcmOyb5u&f5p(t-H z2cMU_sLz6EV%mmMcJZ$;0_Jg!P`g z;X~w87p0Y%%tLOmz&Lk@XYwX~jLOb?DgjfA&F&mVUPAkBIsTjimbGyG*7g0C`=emv zc}&Ub@sjVu<}?+U&g;#ykgx5SJ=OFrK7>lG*^a*Pr3;xC!8sIDwci7Zffl!J^?k(8 zB}WSVJra)J>lJ&}+)S}NJ{~d{iAifc6K8nNO?Em4e9qj?_1Rj z{O0XT^hSk?2h}+G;68BYG{Ah)VY;PGWOqXJj;x;DvD8!MlLHf04CILRk>qm77uK?k zC%8e;k$dpEdj5!?uY7lcAFhsq9bK#th5Bvn7D2}VsYg(jt_#uXBQAl}&Wlha^x1M| zfB1N%e_7q5V*?9xxizoFO1qQ=M||NY&?{0Fsr54y2pZ4;%c4fwX(h@l3^dexVrVWkU50AOQ zOSOk55qHTGy|z_;(94f|B9?P2EwZtilolYiN$%Yqdo(L0SZv9kFDLDK`+r1Z+L(8{ zL&~c7ae@kk*whxx+H#RXcdfqLNl?6u7qVXiMA?LG~yVlCm0a$jOPPVE&zZp_ZLF zU+g{}!Gwm(1n0&lJvikDV*pUNXU3K<54|nnonV%u(76Q?H1P5*xM9Wg^2@1wHZdsE z90LCyppp%}P9tV&I3`5F1fmUV++%`X3OZzcop_On41{%boS;Dm`wn7Ip-0%AOrpAk zB1S?!-mclTe^GF#(-i zANT7NMnC$bENlNPhr_#0$)AT{_q~Wwt6tl-XNHu2CMM9NmBhTHaTN`Sra0`-z?_cX zd|~0=+0jM{!b*wKE`IZUvklaXbmM^la`P=8FZBFxUwLrO&`QnMHX)SEYR;Ckb||0W zJS_#yAdug8mE?OzFwuriG1ua7@6R4k(JzTv&U{_g=;9uNp6GWWWqtkqhk*)(_dT{* z$;1cQHw-uZs1EED9kb-rZ6f&oN_2hZAKfDaVYSq)R#8b$T20Anosfg)+=oY>*ub}M zupxn+U&YE`?o*i{VFVzIZ{{4<$x0#&1eLYk$r&}ToghJn$1<@%_5ujNRP3O@rd4xP z5LP$~^u9-Ia2R4BT}l8#c?|qM`~wTz5P3;5?G57h>+N>PV%3rOl7R|jL1_Pp%-w-{ zaUS9ZuK)o3cc%nwL)opGANfB9lh;y)3WKb~8R2ww+ zfu25+zG^)sL+M848jGIH2M<(-IKq>m34mU>1tt+ranXCDffP3n7n3VaAUJ({GxzO& zd*9{d8;%-;0$}C}kSa$)yqQT&jpK)CoMyK0Yrxqcx4}M7wo9zKxR}B2RaQxffarS- z4c4R1^7Zz+?IWQmc{9W=)G)FI)ChuP&5Qx+MNAp($;nK92URU8h~EO$lG2C8!>sNW zC~<}u!sJ1$9+M^@2Iue#MC-o3jjhhZ!O`cu$hm29-gv0xM;*>xBY2(iVX=e@bQg?J z&4YZ+kFyr>3HoF2M4-Ut>Ol(Q{)o^adOxMD;#=24G~jS#oPdAD@BD;*iTEO%k)6KV z;ZvqJL_+&wi~Q#Pin6aGhdtT+WZ$7-<@@4m=KK1Do&Hnzo0P_!0j_m;Hy7ydPnrFahwT-b@#jae z6-I_Ql709|bOf|J?@f2|*>$}<7>M(%>x!Txq2^}SuhV8Hb$1mIX1?}2L*6TW8*BFElYcI zV?#@8BV(hH-*SHpfyU_gZyh~0S27_y!d+(;g<{!dpZvwMgN8Unm2$gPXJi{#(&lJ zFC4}(2~hky!tbTLNx+-G)A+qaH3g9W7whK|GAv;Vp!zTBqiKM^zo?ee0O^0RCZ_*U zh~F`|XZ{o8PjdM)fZu}(zsmKSMK}wP`Fk|s_fo(t;Pt;E9?t^Q{uR+|4j}$7*5KTK z_Ws|9T7L-s^8l0o;eqV+&yj?G9_(Ku3IDzF{12T0|K9@&ztSe7`swb^$v{A${vYn3 zaRAu68UX13`3w170<;AH1=3JF0QSfQK>cSEzfJ(m?k5IVDThTZ{4Z$YmzntFU!j=( z$8JesVv9eq`zZj7ukL@@75>6Fz5-y5;{fJAb`OgHLM&ngVgx%N0)qM};AeII4@s$a A{r~^~ delta 1017 zcmccdoqO>PZoU9-W)=|!5J>FP2${$yt;u%&W@69fn~5O0w1S&~k>x8R0|QtQL@@^g z2Sjm@Di2Jt*0r07li$zQn7CR;J@LjxgJj0jb?uwJuG(L9uds5fK)Sva14Do}I|p0f z>U*(3%kD5ROjfWFpDfB~!XbM4W+Di04rEld2J>L1OO1ZP5-@~NfFJ0c92~17N+S5mzi`>Y?7Y-9V92ajcK~nRVF1geX+lRa={mw grU!gr($$5z5F@*?vVn58I1tX@WMC-0#02C40P%v)l>h($ diff --git a/tests/test_wrappers/test_qchem.py b/tests/test_wrappers/test_qchem.py index 2356a469c..ef15abbe0 100644 --- a/tests/test_wrappers/test_qchem.py +++ b/tests/test_wrappers/test_qchem.py @@ -546,3 +546,36 @@ def test_butane_grad_extract(): assert np.isclose(flat_grad[-1], 0.0055454, atol=1e-5) assert np.isclose(flat_grad[5], 0.0263383, atol=1e-5) + + +@work_in_zipped_dir(qchem_data_zip_path) +def test_h2_coordinate_extraction_qchem_v6(): + + calc = _blank_calc() + calc.input.keywords = method.keywords.opt + calc.molecule = Molecule(smiles="[H][H]") + calc.set_output_filename("H2_opt_qchem6.out") + + assert np.allclose( + calc.molecule.coordinates.to("Å"), + np.array([[+0.3803762086, 0.0, 0.0], [-0.3803762086, 0.0, 0.0]]), + ) + + +def test_coordinate_extract_from_single_point_calculation(): + + calc = _blank_calc() + calc.input.keywords = method.keywords.sp + calc.molecule = Molecule(smiles="O") + init_coords = calc.molecule.coordinates.copy() + + assert np.allclose(init_coords, QChem().coordinates_from(calc)) + + +def test_coordinate_extract_from_single_atom_calculation(): + + calc = _blank_calc() + calc.input.keywords = method.keywords.opt + calc.molecule = Molecule(atoms=[Atom("H", x=0, y=0, z=0)]) + + assert np.allclose(QChem().coordinates_from(calc), np.zeros(3)) From ef912c252c26890ccd25cd20d9fe1eb874434b89 Mon Sep 17 00:00:00 2001 From: Tom Young <39765193+t-young31@users.noreply.github.com> Date: Wed, 1 Feb 2023 08:43:39 +0000 Subject: [PATCH 4/7] Fix units for values div mul (#242) * Fix units for div mul * Update changelog --- autode/values.py | 33 +++++++++++++++++++++------------ doc/changelog.rst | 2 ++ tests/test_value.py | 12 ++++++++++++ 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/autode/values.py b/autode/values.py index dcf4c0976..295444d53 100644 --- a/autode/values.py +++ b/autode/values.py @@ -62,7 +62,7 @@ def _to(value: Union["Value", "ValueArray"], units: Union[Unit, str]): except StopIteration: raise TypeError( - f"No viable unit conversion from {value.units} " f"-> {units}" + f"No viable unit conversion from {value.units} -> {units}" ) # Convert to the base unit, then to the new units @@ -214,16 +214,22 @@ def __add__(self, other) -> "Value": float(self) + self._other_same_units(other), units=self.units ) - def __mul__(self, other) -> "Value": + def __mul__(self, other) -> Union[float, "Value"]: """Multiply this value with another""" if isinstance(other, np.ndarray): return other * float(self) + if isinstance(other, Value): + logger.warning( + "Multiplying autode.Value returns a float with no units" + ) + return float(self) * self._other_same_units(other) + return self.__class__( float(self) * self._other_same_units(other), units=self.units ) - def __rmul__(self, other) -> "Value": + def __rmul__(self, other) -> Union[float, "Value"]: return self.__mul__(other) def __radd__(self, other) -> "Value": @@ -232,16 +238,19 @@ def __radd__(self, other) -> "Value": def __sub__(self, other) -> "Value": return self.__add__(-other) - def __floordiv__(self, other): - raise NotImplementedError( - "Integer division is not supported by " "autode.values.Value" - ) + def __floordiv__(self, other) -> Union[float, "Value"]: + x = float(self) // self._other_same_units(other) + if isinstance(other, Value): + return x - def __truediv__(self, other) -> "Value": - return self.__class__( - float(self) / float(self._other_same_units(other)), - units=self.units, - ) + return self.__class__(x, units=self.units) + + def __truediv__(self, other) -> Union[float, "Value"]: + x = float(self) / self._other_same_units(other) + if isinstance(other, Value): + return x + + return self.__class__(x, units=self.units) def __abs__(self) -> "Value": """Absolute value""" diff --git a/doc/changelog.rst b/doc/changelog.rst index ab9c69ad7..8303c1361 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -19,6 +19,8 @@ Functionality improvements Bug Fixes ********* - Fixes :code:`ERROR` logging level being ignored from environment variable :code:`AUTODE_LOG_LEVEL` +- Fixes :code:`autode.values.Value` instances generating items with units on division, and throw a warning if multiplying + 1.3.4 -------- diff --git a/tests/test_value.py b/tests/test_value.py index 924939e68..d2d23a072 100644 --- a/tests/test_value.py +++ b/tests/test_value.py @@ -266,3 +266,15 @@ class Tmp: with pytest.raises(Exception): _to(Tmp(), units="Å") + + +def test_div_mul_generate_floats(): + + e = PotentialEnergy(1.0) + assert isinstance(e / e, float) + assert isinstance(e // e, float) + + assert e // e == 1 + + # Note: this behaviour is not ideal. But it is better than having the wrong units + assert isinstance(e * e, float) From 0d03666aa291471107be3439ab396fb6e5497d52 Mon Sep 17 00:00:00 2001 From: Tom Young <39765193+t-young31@users.noreply.github.com> Date: Wed, 1 Feb 2023 10:52:02 +0000 Subject: [PATCH 5/7] Update workflow (#244) --- .github/workflows/pytest_cov.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/pytest_cov.yml b/.github/workflows/pytest_cov.yml index b797805f5..3fb86b6a4 100644 --- a/.github/workflows/pytest_cov.yml +++ b/.github/workflows/pytest_cov.yml @@ -4,6 +4,7 @@ on: push: branches: - master + - 'v[0-9]+.[0-9]+.[0-9]+' pull_request: paths-ignore: - 'doc/**' From 4276cabb57a7d8fddace6f95de43217c9a9fda3e Mon Sep 17 00:00:00 2001 From: Tom Young <39765193+t-young31@users.noreply.github.com> Date: Sat, 11 Feb 2023 10:03:00 +0000 Subject: [PATCH 6/7] Fix xtb opt with cartesian coordinates (#247) * Fix xtb opt with cartesian coordinates * Update changelog [skip actions] --- autode/wrappers/XTB.py | 34 +++++++++++++++++---------------- doc/changelog.rst | 1 + tests/test_wrappers/test_xtb.py | 25 ++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/autode/wrappers/XTB.py b/autode/wrappers/XTB.py index 3edc72edc..dfd88626f 100644 --- a/autode/wrappers/XTB.py +++ b/autode/wrappers/XTB.py @@ -50,29 +50,31 @@ def print_cartesian_constraints(self, inp_file, molecule): if molecule.constraints.cartesian is None: return None - constrained_atom_idxs = [i + 1 for i in molecule.constraints.cartesian] - list_of_ranges, used_atoms = [], [] - - for i in constrained_atom_idxs: - atom_range = [] - if i not in used_atoms: - while i in constrained_atom_idxs: - used_atoms.append(i) - atom_range.append(i) - i += 1 - if len(atom_range) in (1, 2): - list_of_ranges += str(atom_range) - else: - list_of_ranges.append(f"{atom_range[0]}-{atom_range[-1]}") + atom_idxs = list( + sorted(int(i) + 1 for i in molecule.constraints.cartesian) + ) + list_of_ranges = [] + for atom_idx in atom_idxs: + last_range = ( + list_of_ranges[-1] if len(list_of_ranges) > 0 else None + ) + if last_range is not None and atom_idx - 1 == last_range[-1]: + last_range.append(atom_idx) + else: + list_of_ranges.append([atom_idx]) + + list_of_ranges_str = [ + f"{idxs[0]}-{idxs[-1]}" if len(idxs) > 1 else str(idxs[0]) + for idxs in list_of_ranges + ] print( f"$constrain\n" f"force constant={self.force_constant}\n" - f'atoms: {",".join(list_of_ranges)}\n' + f'atoms: {",".join(list_of_ranges_str)}\n' f"$", file=inp_file, ) - return None @staticmethod diff --git a/doc/changelog.rst b/doc/changelog.rst index 8303c1361..d6144a7ee 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -20,6 +20,7 @@ Bug Fixes ********* - Fixes :code:`ERROR` logging level being ignored from environment variable :code:`AUTODE_LOG_LEVEL` - Fixes :code:`autode.values.Value` instances generating items with units on division, and throw a warning if multiplying +- Fixes the printing of cartesian constraints in XTB input files, meaning they are no longer ignored 1.3.4 diff --git a/tests/test_wrappers/test_xtb.py b/tests/test_wrappers/test_xtb.py index 5430a7f65..b2a86de7a 100644 --- a/tests/test_wrappers/test_xtb.py +++ b/tests/test_wrappers/test_xtb.py @@ -302,6 +302,11 @@ def test_xtb_opt_non_contiguous_range_cart_constraints(): ) calc.run() + assert len(calc.input.additional_filenames) > 0 + xcontrol_lines = open(calc.input.additional_filenames[-1], "r").readlines() + expected_range = "1-3,6" + assert sum(expected_range in line for line in xcontrol_lines) == 1 + assert calc.optimiser.converged assert mol.energy is not None @@ -362,3 +367,23 @@ def run_calc(_mol): run_calc(mol) assert mol.energy != unconstrained_energy + + +@testutils.requires_with_working_xtb_install +@work_in_tmp_dir() +def test_xtb_cartesian_constrained_opt(): + + init_r = 0.9 + h2 = Molecule(atoms=[Atom("H"), Atom("H", x=init_r)]) + + h2_unconstrained = h2.new_species(name="unconstrained_h2") + h2_unconstrained.optimise(method=XTB()) + # expected minimum for H2 is ~0.77 Å + assert abs(h2_unconstrained.distance(0, 1) - init_r) > 0.1 + + h2.constraints.cartesian = [0, 1] + h2.optimise(method=XTB()) + + # if the coordinates are constrained then the distance should be + # close to the initial + assert abs(h2.distance(0, 1) - init_r) < 0.1 From dded76bfb77165f9ceb039f7d75d659b02321c85 Mon Sep 17 00:00:00 2001 From: Tom Young <39765193+t-young31@users.noreply.github.com> Date: Sat, 11 Feb 2023 10:24:20 +0000 Subject: [PATCH 7/7] Remove hessian conversion on normal mode calculation (#243) * Ensure no unit conversion in hessian * Use a copy for value and valuearray * Raise exception on value `to` with inplace * Add missed changelog updates --- autode/hessians.py | 6 ++-- autode/units.py | 6 ++-- autode/values.py | 75 +++++++++++++++++++++++++++---------------- doc/changelog.rst | 9 +++--- tests/test_hessian.py | 9 +++++- tests/test_value.py | 23 +++++++++++++ tests/test_values.py | 20 +++++++++++- 7 files changed, 108 insertions(+), 40 deletions(-) diff --git a/autode/hessians.py b/autode/hessians.py index b3f6149a2..4b8cc7616 100644 --- a/autode/hessians.py +++ b/autode/hessians.py @@ -234,9 +234,9 @@ def _mass_weighted(self) -> np.ndarray: axis=np.newaxis, ) - return Hessian( - H / np.sqrt(np.outer(mass_array, mass_array)), units="J m^-2 kg^-1" - ) + return np.array( + H / np.sqrt(np.outer(mass_array, mass_array)) + ) # J Å^-2 kg^-1 @cached_property def _proj_mass_weighted(self) -> np.ndarray: diff --git a/autode/units.py b/autode/units.py index 8f1881881..4b30a1d22 100644 --- a/autode/units.py +++ b/autode/units.py @@ -211,15 +211,15 @@ def energy_unit_from_name(name: str) -> "autode.units.Unit": ha_per_ang = CompositeUnit( - ha, per=[ang], aliases=["ha Å-1", "ha Å^-1", "ha/ang"] + ha, per=[ang], aliases=["ha / Å", "ha Å-1", "ha Å^-1", "ha/ang"] ) ha_per_a0 = CompositeUnit( - ha, per=[a0], aliases=["ha a0-1", "ha a0^-1", "ha/bohr"] + ha, per=[a0], aliases=["ha / a0", "ha a0-1", "ha a0^-1", "ha/bohr"] ) ev_per_ang = CompositeUnit( - ev, per=[ang], aliases=["ha a0-1", "ev Å^-1", "ev/ang"] + ev, per=[ang], aliases=["ev / Å", "ev Å^-1", "ev/ang"] ) kcalmol_per_ang = CompositeUnit( diff --git a/autode/values.py b/autode/values.py index 295444d53..77fc41ca1 100644 --- a/autode/values.py +++ b/autode/values.py @@ -37,9 +37,12 @@ ) -def _to(value: Union["Value", "ValueArray"], units: Union[Unit, str]): +def _to( + value: Union["Value", "ValueArray"], units: Union[Unit, str], inplace: bool +) -> Any: """ - Convert a value or value array to a new unit and return a copy + Convert a value or value array to a new unit and return a copy if + inplace=False --------------------------------------------------------------------------- Arguments: @@ -65,23 +68,26 @@ def _to(value: Union["Value", "ValueArray"], units: Union[Unit, str]): f"No viable unit conversion from {value.units} -> {units}" ) - # Convert to the base unit, then to the new units - c = float(units.conversion / value.units.conversion) - - if isinstance(value, Value): - return value.__class__(float(value) * c, units=units) - - elif isinstance(value, ValueArray): - value[:] = np.array(value, copy=True) * c - value.units = units - return value - - else: + if not (isinstance(value, Value) or isinstance(value, ValueArray)): raise ValueError( f"Cannot convert {value} to new units. Must be one of" f" Value of ValueArray" ) + if isinstance(value, Value) and inplace: + raise ValueError( + "Cannot modify a value inplace as floats are immutable" + ) + + # Convert to the base unit, then to the new units + c = float(units.conversion / value.units.conversion) + + new_value = value if inplace else value.copy() + new_value *= c + new_value.units = units + + return None if inplace else new_value + def _units_init(value, units: Union[Unit, str, None]): """Initialise the units of this value @@ -171,6 +177,11 @@ def _other_same_units(self, other): return other.to(self.units) + def _like_self_from_float(self, value: float) -> "Value": + new_value = self.__class__(value, units=self.units) + new_value.__dict__.update(self.__dict__) + return new_value + def __eq__(self, other) -> bool: """Equality of two values, which may be in different units""" @@ -210,8 +221,8 @@ def __add__(self, other) -> "Value": if isinstance(other, np.ndarray): return other + float(self) - return self.__class__( - float(self) + self._other_same_units(other), units=self.units + return self._like_self_from_float( + float(self) + self._other_same_units(other) ) def __mul__(self, other) -> Union[float, "Value"]: @@ -225,8 +236,8 @@ def __mul__(self, other) -> Union[float, "Value"]: ) return float(self) * self._other_same_units(other) - return self.__class__( - float(self) * self._other_same_units(other), units=self.units + return self._like_self_from_float( + float(self) * self._other_same_units(other) ) def __rmul__(self, other) -> Union[float, "Value"]: @@ -240,17 +251,11 @@ def __sub__(self, other) -> "Value": def __floordiv__(self, other) -> Union[float, "Value"]: x = float(self) // self._other_same_units(other) - if isinstance(other, Value): - return x - - return self.__class__(x, units=self.units) + return x if isinstance(other, Value) else self._like_self_from_float(x) def __truediv__(self, other) -> Union[float, "Value"]: x = float(self) / self._other_same_units(other) - if isinstance(other, Value): - return x - - return self.__class__(x, units=self.units) + return x if isinstance(other, Value) else self._like_self_from_float(x) def __abs__(self) -> "Value": """Absolute value""" @@ -269,7 +274,7 @@ def to(self, units): Raises: (TypeError): """ - return _to(self, units) + return _to(self, units, inplace=False) class Energy(Value): @@ -652,7 +657,21 @@ def to(self, units) -> Any: Raises: (TypeError): """ - return _to(self, units) + return _to(self, units, inplace=False) + + def to_(self, units) -> None: + """ + Convert this array into a set of new units, inplace. This will not copy + the array + + ----------------------------------------------------------------------- + Returns: + (None) + + Raises: + (TypeError): + """ + _to(self, units, inplace=True) def __array_finalize__(self, obj): """See https://numpy.org/doc/stable/user/basics.subclassing.html""" diff --git a/doc/changelog.rst b/doc/changelog.rst index d6144a7ee..23be882ce 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -8,12 +8,12 @@ Changelog Usability improvements/Changes ****************************** -* +- :code:`autode.value.ValueArray.to()` now defaults to copying the object rather than inplace modification Functionality improvements ************************** -- +- Adds a :code:`to_` method to :code:`autode.value.ValueArray` for explicit inplace modification of the array Bug Fixes @@ -21,6 +21,8 @@ Bug Fixes - Fixes :code:`ERROR` logging level being ignored from environment variable :code:`AUTODE_LOG_LEVEL` - Fixes :code:`autode.values.Value` instances generating items with units on division, and throw a warning if multiplying - Fixes the printing of cartesian constraints in XTB input files, meaning they are no longer ignored +- Fixes :code:`Hessian` instances changing units when normal modes are calculated +- Fixes an incorrect alias for :code:`ev_per_ang` 1.3.4 @@ -32,7 +34,6 @@ Feature additions. Usability improvements/Changes ****************************** * Throw useful exception for invalid :code:`ade.Config.ts_template_folder_path` -* Adds the reaction temperature to the unique reaction hash Functionality improvements @@ -44,7 +45,7 @@ Functionality improvements Bug Fixes ********* -- +- Fixes calculation :code:`clean_up()` failing with a null filename 1.3.3 diff --git a/tests/test_hessian.py b/tests/test_hessian.py index c611c22e4..554a4c2be 100644 --- a/tests/test_hessian.py +++ b/tests/test_hessian.py @@ -13,7 +13,7 @@ from autode.species import Molecule from autode.values import Frequency from autode.geom import calc_rmsd -from autode.units import wavenumber +from autode.units import wavenumber, ha_per_ang_sq from autode.exceptions import CalculationException from autode.wrappers.keywords import pbe0 from autode.transition_states.base import displaced_species_along_mode @@ -243,6 +243,7 @@ def assert_correct_co2_frequencies(hessian, expected=(666, 1415, 2517)): """Ensure the projected frequencies of CO2 are roughly right""" nu_1, nu_2, nu_3 = expected + print(hessian.frequencies_proj) assert sum(freq == 0.0 for freq in hessian.frequencies_proj) == 5 # Should have a degenerate bending mode for CO2 with ν = 666 cm-1 @@ -419,6 +420,7 @@ def test_hessian_modes(): h2o = Molecule("H2O_hess_orca.xyz") h2o.hessian = h2o_hessian_arr + assert h2o.hessian.units == ha_per_ang_sq # The structure is a minimum, thus there should be no imaginary frequencies assert h2o.imaginary_frequencies is None @@ -441,6 +443,9 @@ def test_hessian_modes(): vib_mode, h2o.hessian.normal_modes[6 + i], atol=0.1 ) or np.allclose(vib_mode, -h2o.hessian.normal_modes[6 + i], atol=0.1) + # Hessian units should be retained + assert h2o.hessian.units == ha_per_ang_sq + @testutils.work_in_zipped_dir(os.path.join(here, "data", "hessians.zip")) def test_proj_modes(): @@ -595,6 +600,8 @@ def test_nwchem_hessian_co2(): keywords=ade.HessianKeywords(), ) calc.set_output_filename("CO2_hess_nwchem.out") + print(co2.hessian) + print(co2.hessian._mass_weighted) assert_correct_co2_frequencies( hessian=co2.hessian, expected=(659.76, 1406.83, 2495.73) ) diff --git a/tests/test_value.py b/tests/test_value.py index d2d23a072..cf115e9f9 100644 --- a/tests/test_value.py +++ b/tests/test_value.py @@ -3,6 +3,7 @@ from autode.constants import Constants from autode.units import ha, kjmol, kcalmol, ev, ang, a0, nm, pm, m, rad, deg from autode.values import ( + _to, Value, Distance, MWDistance, @@ -278,3 +279,25 @@ def test_div_mul_generate_floats(): # Note: this behaviour is not ideal. But it is better than having the wrong units assert isinstance(e * e, float) + + +def test_operations_maintain_other_attrs(): + + e = Energy(1, estimated=True, units="eV") + assert e.is_estimated and e.units == ev + + e *= 2 + assert e.is_estimated and e.units == ev + + e /= 2 + assert e.is_estimated and e.units == ev + + a = e * 2 + assert a.is_estimated and e.units == ev + + +def test_inplace_value_modification_raises(): + + e = Energy(1, units="Ha") + with pytest.raises(ValueError): # floats are immutable + _to(e, units="eV", inplace=True) diff --git a/tests/test_values.py b/tests/test_values.py index cac6882a3..dd8059111 100644 --- a/tests/test_values.py +++ b/tests/test_values.py @@ -106,4 +106,22 @@ class InvalidValue(float): def test_to_unsupported(): with pytest.raises(ValueError): - _ = _to(InvalidValue(), Unit()) + _ = _to(InvalidValue(), Unit(), inplace=True) + + +def test_inplace_modification(): + + x = Gradient([[1.0, 1.0, 1.0]], units="Ha / Å") + return_value = x.to_("eV / Å") + assert return_value is None + + assert not np.allclose(x, np.ones(shape=(1, 3))) + + +def test_copy_conversion(): + + x = Gradient([[1.0, 1.0, 1.0]], units="Ha / Å") + y = x.to("eV / Å") + + assert not np.allclose(x, y) + assert np.allclose(x, np.ones(shape=(1, 3)))