Fix gfnff printout for oniom and add $ignore topo #783
Conversation
| if (pr) then | ||
| call timer%new(10 + count([allocated(solvation)]),.false.) | ||
| else | ||
| call timer%new(1, .false.) |
src/gfnff/gfnff_eg.f90
Outdated
| @@ -612,7 +615,10 @@ subroutine gfnff_eg(env,pr,n,ichrg,at,xyz,makeq,g,etot,res_gff, & | |||
| ! which is computed here to get the atomization energy De,n,at(n) | |||
| call goed_gfnff(env,.true.,n,at,sqrab,srab,dfloat(ichrg),eeqtmp,cn,qtmp,eesinf,solvation,param,topo) | |||
| de=-(etot - eesinf) | |||
| else | |||
| call timer%write(6) | |||
There was a problem hiding this comment.
This one is added to print the calculation time for a single cycle during the gfnff geometry optimization.
There was a problem hiding this comment.
How about a minpr level compared to a full pr level of verbosity? (better variable names highly appreciated)
There was a problem hiding this comment.
What do you think about adding an optional argument to the gfnff_eg only for the case of optimization?
Otherwise, I do not see the reason for changing pr into minpr (or maybe optpr as an alternative name).
There was a problem hiding this comment.
I have to agree with @awvwgk here. I don't see this as a necessary default printout and would prefer if this would only be written if verbose is specified.
Additionally, it would be better if we make it more clear, that this is the single cycle time. total time might be misleading.
There was a problem hiding this comment.
So I added minpr/optpr option for geometry optimization + changed total time to iter.time for clarity.
Btw, I think we should reassign global printlevel var to TSet class.
Signed-off-by: albert <92109627+Albkat@users.noreply.github.com>
Signed-off-by: albert <92109627+Albkat@users.noreply.github.com>
Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>
Signed-off-by: albert <92109627+Albkat@users.noreply.github.com>
Signed-off-by: albert <92109627+Albkat@users.noreply.github.com>
src/gfnff/gfnff_eg.f90
Outdated
| if (pr) call timer%new(10 + count([allocated(solvation)]),.false.) | ||
| if (pr) then | ||
| call timer%new(10 + count([allocated(solvation)]),.false.) | ||
| else if (present(minpr) .and. minpr) then |
There was a problem hiding this comment.
You cannot check for presence of an optional argument and use the argument in the same condition as the argument might not be present and the order of evaluation is not guaranteed.
src/type/timer.f90
Outdated
| !> dummy arg list | ||
| class(tb_timer),intent(inout) :: self | ||
| !! instance of timer |
There was a problem hiding this comment.
Please be consistent, either add all comments before or after the variable name. In xtb I have been using comments before the variables so far.
src/type/timer.f90
Outdated
| call self%deallocate | ||
| if (n < 1) return | ||
| call timing(time_cpu,time_wall) | ||
| !! capture negative values |
There was a problem hiding this comment.
Both !! and !> create a docstring for tools like FORD or doxygen, these tools usually get confused if variables and statements are documented this way which are not part of the interface of the procedure.
There was a problem hiding this comment.
All right, I checked the FORD documentation, and see your point.
I try to be consistent within the source using only predocmark (!>) .
Did you try to generate xtb-docstring using FORD?
There was a problem hiding this comment.
Yeah, I tried a while ago, but never deployed because we don't had a good coverage of docstrings so far. Still good to work toward the goal of deploying the whole docs in the future like for tblite.
There was a problem hiding this comment.
Sounds like a lot of fun ;)
There was a problem hiding this comment.
I rewrote all my comments in a consistent style, so I think this PR is good to go.
It will be helpful to establish some style guide in the best case directly in GitHub, but I am not aware if it is possible. Do you know any means of doing that?
Another possibility would be just add here a developer guide.
Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>
Signed-off-by: albert <92109627+Albkat@users.noreply.github.com>
* add the 'ignore topo' in the control Signed-off-by: albert <92109627+Albkat@users.noreply.github.com> * fix gfnff printout for oniom opt Signed-off-by: albert <92109627+Albkat@users.noreply.github.com> * Update calculator.f90 * Update src/oniom.f90 Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com> * add minpr to print iter time during gfnff geo_opt Signed-off-by: albert <92109627+Albkat@users.noreply.github.com> * clear timer.f90 Signed-off-by: albert <92109627+Albkat@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com> * consistent comments: oniom.f90, gfnff_eg.f90, timer.f90 Signed-off-by: albert <92109627+Albkat@users.noreply.github.com> * Update .gitignore --------- Signed-off-by: albert <92109627+Albkat@users.noreply.github.com> Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>
gfn2:gfnffprintout for structure optimization by printing energies and gradients during each cycle$ignore topoinstruction to bypass the topology check when cutting bondsgfnff_eg.f90,timer.f90,oniom.f90