Support dipole moments in ORCA (taken over !3020)
This MR is taken over from !3020 (closed) (made by @jfojt; thank you so much for the nice addition!) to add a dipole-moment parser for the ORCA output.
There are several remaining issues to be fixed in !3020 (closed), which are addressed in the present MR:
- There are 3 merge commits (9f47df72, d1e998c6, 3eb28afa) including inherited changes on several hundreds of files. These may be better not to be included in the master-branch history, and for that we probably need
git rebase
. - As commented by @askhl (here), it should nowadays be avoided to add a big file to
testdata
, and it is instead now recommended to use a short string snippet to test a new functionality. To not inherit the big file in the commits in !3020 (closed), here it is also required to dogit rebase
.
In the beginning I wanted to simply update !3020 (closed) (following the guidance of Gitlab) via the forked repository jfojt/ase:fix-orca
, but I found that probably I cannot update it when I did a full rebase. I therefore needed to open this as this new MR.
@askhl @AndrewRosen I would be very happy if you kindly review if this MR makes sense.
Closes !3020 (closed) (I will do by hand if the present MR is accepted.)
Checklist
-
I am familiar with ASE's contribution guidelines. -
Doc strings in code changed in this MR are up to date. -
Unit tests have been added for new or changed code. -
Issue is resolved via "closes #XXXX" if applicable.
Merge request reports
Activity
I will be happy to review this. Would you be willing to review !3352 (merged) in return?
Thank you, @yuzie007! This all looks quite reasonable to me. The only comment I have is if we should be modifying
read_orca_forces
(nowread_orca_engrad
). I agree that the name change is better, but do we want to bother with changing the precise substring searches if the original approach was not broken? Switching to regex is appealing but at the same time, we have to be careful not to be too strict because developers (especially ORCA developers) love to make minor tweaks to the formatting of the log file...Thank you @AndrewRosen for the check!
I agree to reduce the
re
usage. Apart from the potential too-much strictness, it may usually be a bit more difficult to read, and also there may be potential disadvantages on performance (StackOverflow). I have just now revised the relevant commits.
added 7 commits
Toggle commit listadded 7 commits
Toggle commit list97 continue 98 if getgrad and "#" not in line: 99 grad = line.split()[-1] 100 tempgrad.append(float(grad)) 101 if len(tempgrad) == 3: 102 gradients.append(tempgrad) 103 tempgrad = [] 104 if '# The at' in line: 105 getgrad = False 106 107 forces = -np.array(gradients) * Hartree / Bohr 154 fd.readline() # skip line only with '#' 155 gradients = [float(fd.readline()) for _ in range(3 * natoms)] 156 break 157 gradients = np.array(gradients).reshape((-1, 3)) 158 forces = -1.0 * gradients * Hartree / Bohr - Comment on lines +145 to +158
@yuzie007: Just one final question before proceeding. What was the motivation for updating the
read_orca_engrad
function? Is this fixing some functionality, is it more robust, faster, just for refactoring's sake, etc?Since the original scope of the MR is about dipole moments, I want to make sure the motivation behind any other decisions are known.
changed this line in version 4 of the diff
Thank you @AndrewRosen for the check!
This was just a refactoring (which for myself was easy to read), but I admit that I did not do a careful and objective performance check. I now reverted to the original implementation. I fully agree that it is better here to focus on the dipole-moment parser, and if in future we found the necessity of refactoring, we can open a separate MR.
Thank you, @yuzie007! I appreciate it, even if it meant reverting some of your work. If potential issues arise, this separation of work helps ensure that it is easier to identify the change that breaks things. For something as crucial as force parsing, I felt keeping it separate would be worthwhile.
added 7 commits
Toggle commit listmentioned in commit a4b1986e
mentioned in merge request !3020 (closed)