Closed Brakjen closed 2 years ago
Merging #408 (bd52a5b) into master (2d0f602) will increase coverage by
0.15%
. The diff coverage is82.35%
.
@@ Coverage Diff @@
## master #408 +/- ##
==========================================
+ Coverage 68.04% 68.20% +0.15%
==========================================
Files 182 183 +1
Lines 15208 15453 +245
==========================================
+ Hits 10349 10540 +191
- Misses 4859 4913 +54
Impacted Files | Coverage Δ | |
---|---|---|
src/chemistry/Molecule.cpp | 52.66% <0.00%> (-14.26%) |
:arrow_down: |
src/chemistry/Molecule.h | 83.33% <ø> (-2.88%) |
:arrow_down: |
src/chemistry/PhysicalConstants.cpp | 40.00% <0.00%> (-10.00%) |
:arrow_down: |
src/parallel.cpp | 80.43% <ø> (ø) |
|
src/qmoperators/one_electron/ZoraOperator.h | 100.00% <ø> (ø) |
|
src/qmoperators/two_electron/ReactionPotential.h | 100.00% <ø> (ø) |
|
...c/qmoperators/two_electron/ExchangePotentialD2.cpp | 45.00% <33.33%> (ø) |
|
...rc/qmoperators/two_electron/CoulombPotentialD1.cpp | 50.00% <50.00%> (ø) |
|
...rc/qmoperators/two_electron/CoulombPotentialD2.cpp | 53.84% <50.00%> (ø) |
|
src/tensor/RankZeroOperator.cpp | 64.46% <50.00%> (ø) |
|
... and 31 more |
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 2d0f602...bd52a5b. Read the comment docs.
Where is the appropriate place to print information about SCRF and the cavity? Now I am printing in driver.cpp
when setting up ReactionOperator
, since this block only gets executed if a reaction field is requested.
if (json_fock.contains("reaction_operator")) {
// preparing Reaction Operator
(...)
mol.printCavity();
V_R->getHelper()->printParameters();
}
printCavity()
can perhaps be done at the same time as printGeometry()
, after initCavity()
in the molecule? But what happens here if we're not doing solvent, do we still get a cavity?
Ok, we need to fix this, there should not be any cavity_coords
input section in the molecule
when we don't have any solvent. This needs to be screened out in the Python part.
Ok, we need to fix this, there should not be any
cavity_coords
input section in themolecule
when we don't have any solvent. This needs to be screened out in the Python part.
This will interfere a bit with PR #404, since the molecule and cavity validation has been abstracted to its own validator class. How best proceed? (which PR will be merged first?:P)
Yes, I see the JSON printer was also introduced in the other PR, so let's finish that one first. Can you move the final version of the JSON printer over to #404, then I can have a look at the Cavity stuff after lunch. Not sure where to put it yet
If you first squash
these into a single commit, then a rebase
on the new upstream
will result in only trivial conflicts.
We should also fix some of the solvent related printouts:
print_level > 1
does not fit well with the rest===========================================================================
Molecular Energy (final)
---------------------------------------------------------------------------
Kinetic energy : (au) 67.700399048617
E-N energy : (au) -181.201728318483
Coulomb energy : (au) 37.493587268261
Exchange energy : (au) 0.000000000000
X-C energy : (au) -7.627268432947
Ext. field (el) : (au) 0.000000000000
---------------------------------------------------------------------------
N-N energy : (au) 9.039235444235
Ext. field (nuc) : (au) 0.000000000000
Tot. Reac. Energy : (au) -6.737278530459
El. Reac. Energy : (au) 5.062225603410
Nuc. Reac. Energy : (au) -11.799504133868
---------------------------------------------------------------------------
Electronic energy : (au) -78.572784831142
Nuclear energy : (au) -2.760268689633
---------------------------------------------------------------------------
Total energy : (au) -8.133305352077e+01
: (kcal/mol) -5.103726163876e+04
: (kJ/mol) -2.135399026966e+05
: (eV) -2.213185133919e+03
===========================================================================
Now printCavity()
can be put in driver::init_molecule
, after the cavity has been initialized (and move printGeometry()
above the cavity block).
Not sure where the SCRF output should be though, but these are "parameters" of the calculation and not "program output", so the SCRF print should be enclosed in
~~~~~~~
~~~~~~~
and not
========
========
Actually, the "Physical Constants" should perhaps also be ~
If you first
squash
these into a single commit, then arebase
on the newupstream
will result in only trivial conflicts.
Before I mess everything up: Is this what I should do?
First do a
git rebase -i 8ae9f19d
and change pick
to squash
for all commits except the top one in the commit file?
And then do a
git rebase upstream/master
and resolve conflicts?
And then a
git push --force
?
Sounds about right, starting with a git fetch upstream
. Could be a good idea to make a backup branch with the current state in case something :boom: in your face :smile_cat:
I'll give it a 10% chance that I did the rebase correctly...
Looks like the print_utils::json
got duplicated
Is it okay to just fix this "manually" by adding new commits.
The physical constants printouts should look like this
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
angstrom2bohrs : 1.8897261246257702
dipmom_au2debye : 2.5417464739297717
electron_g_factor : -2.00231930436256
fine_structure_constant : 0.0072973525693
hartree2ev : 27.211386245988
hartree2kcalmol : 627.5094740630558
hartree2kjmol : 2625.4996394798254
hartree2simagnetizability : 78.9451185
hartree2wavenumbers : 219474.6313632
light_speed : 137.035999084
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
or with a headline
***************************************************************************
*** ***
*** Physical Constants ***
*** ***
***************************************************************************
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
angstrom2bohrs : 1.8897261246257702
dipmom_au2debye : 2.5417464739297717
electron_g_factor : -2.00231930436256
fine_structure_constant : 0.0072973525693
hartree2ev : 27.211386245988
hartree2kcalmol : 627.5094740630558
hartree2kjmol : 2625.4996394798254
hartree2simagnetizability : 78.9451185
hartree2wavenumbers : 219474.6313632
light_speed : 137.035999084
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Sorry, I dropped out a bit here :slightly_smiling_face: I think a headline is perhaps a bit excessive?
I checked, and the PCM part is not active during the initial guess.
Since the parameters for the SCF and for the SCRF perhaps should be printed next to each other, it might be clearer to indicate what the tables contain. So something like this? (trying to keep the tilde-style for parameters more or less consistent):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Self-Consistent Field
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Calculation : Optimize ground state orbitals
Method : DFT (PBE0)
Relativity : None
Environment : PCM
Checkpointing : Off
Max iterations : 100
KAIN solver : 5
Localization : Off
Diagonalization : First two iterations
Start precision : 1.00000e-03
Final precision : 1.00000e-03
Helmholtz precision : Dynamic
Energy threshold : Off
Orbital threshold : 1.00000e-01
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Self-Consistent Reaction Field
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Accelerate with KAIN : true
Algorithm : scrf
Convergence criterion : dynamic
Density type : total
Dielec. const. (in) : 1.0
Dielec. const. (out) : 2.0
Max. iterations : 100
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@stigrj
- I don't like the final reaction energy print
What exactly should be improved? :)
We now get many new energy contributions that are printed in the end even if they are not relevant. Same with the upcoming PBC addition. We should be printing only non-zero terms, at least for the non standard contributions.
I like the new parameter output :+1: I wonder if the dielectric constants rather belong in the cavity part of the output? The problem is, they are not immediately connected in the code. We have the Cavity
which is a pure geometric thing, which is combined with the dielectric constants into a physical Permittivity
, which is then used to build the ReactionPotential
in the SCRF
.
Perhaps it's the Permittivity
that should be printed, which would contain a Cavity
, the dielectric constants and the "formulation" ("exponential" seems to be the only one implemented, but could still be stated in the output). And then we can print this Permittivity
together with the SCRF
optimization parameters just after their construction, before we start the ground-state SCF (instead of printing the Cavity
up front together with the molecular geometry).
@stigrj:
Perhaps it's the
Permittivity
that should be printed, which would contain aCavity
, the dielectric constants and the "formulation" ("exponential" seems to be the only one implemented, but could still be stated in the output). And then we can print thisPermittivity
together with theSCRF
optimization parameters just after their construction, before we start the ground-state SCF (instead of printing theCavity
up front together with the molecular geometry).
I have moved the printing to Permittivity.cpp
, and the table now looks like this:
===========================================================================
Solvation Cavity
---------------------------------------------------------------------------
Cavity width : 0.500000
Dielec. Const. (in) : 1.000000
Dielec. Const. (out) : 2.000000
Formulation : exponential
---------------------------------------------------------------------------
N Radius : x y z
---------------------------------------------------------------------------
0 4.000000 : 0.000000 0.000000 0.000000
===========================================================================
But when printing these just after construction, which would be in driver::build_fock_operator
, their placement in the output file does not seem optimal to me. For example, the Li test output looks like this:
It is not completely clear in this output that the SCRF parameters are irrelevant for the initial guess, and that the solvent algorithms are only used for the subsequent SCF. Also, if the SCF::run
keyword is set to false
, the headline is still printed along with the (now completely irrelevant) SCRF and cavity parameters.
If I move the SCRF and cavity printouts to GroundStateSolver::optimize
, and slightly modify the headline
s in driver::scf::run
, the output becomes:
This also plays well with higher print levels, since the parameters are printed right after the headline before any output from the calculation is printed.
Kind of forgot about this. What's the status here? I agree your latter example looks better
Actually, after looking more closely on this today, I think the correct thing is to print the Permittivity/Cavity information up front, and at the same time include solvent effect in the initial guess energy. This would be more in line with the rest of the terms.
For the initial guess, the orbitals are always "given" by the chosen method (previous mw
calculation, sad
guess, gto
guess, etc), i.e. the solvent is not included (and should not be) when computing these initial orbitals. However, the initial energy should be computed with the "correct" electronic method using these given orbitals, i.e. solvent effects should be included here.
@Gabrielgerez why did we disable solvent effects on the first_iteration
when setting up the ReactionPotential
? I guess it had to do with some convergence issue with a poor guess density, but I don't see how it would make a difference to postpone the first SCRF from the initial guess and into the first SCF calculation. The first SCRF convergence should be much the same, no?
I will add some of these changes to this branch tomorrow.
For now the solvent effect is not added to the initial energy, but I will make a follow-up PR where I suggest to change this.
Bottom line:
print_level
that exposes some details on this part. This now happens at print_level=3
and looks like this:
===========================================================================
Building Reaction operator
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Density type : total
Dynamic threshold : On
KAIN solver : 5
Max iterations : 100
Method : SCRF
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Precision (rel) 1.00000e-05
Threshold (abs) 4.79355e-02
---------------------------------------------------------------------------
Vacuum density 1256 nds 55.88 MB 0.28 sec
Vacuum potential 712 nds 31.68 MB 1.43 sec
Initial gamma 2312 nds 102.87 MB 0.31 sec
---------------------------------------------------------------------------
Iter 1 : 1.775248394621e+01 3.457329e+00 1.89 sec
Iter 2 : 2.656614712073e+01 9.020400e+00 1.86 sec
Iter 3 : 1.429949183650e+01 1.257527e+01 2.11 sec
Iter 4 : 1.970794345167e+01 1.718968e+00 1.96 sec
Iter 5 : 2.174428071669e+01 1.207012e+00 2.10 sec
Iter 6 : 2.037670045217e+01 8.438552e-01 2.29 sec
Iter 7 : 2.097124673548e+01 8.051072e-03 2.30 sec
---------------------------------------------------------------------------
Reaction potential 1400 nds 62.29 MB 14.51 sec
---------------------------------------------------------------------------
Wall time: 1.65223e+01 sec
===========================================================================
Also rearranged the molecular energy output to this, with a similar block for external field terms:
===========================================================================
Molecular Energy (final)
---------------------------------------------------------------------------
Kinetic energy : (au) 88.754508805409
E-N energy : (au) -250.283910448742
Coulomb energy : (au) 62.007501412212
Exchange energy : (au) 0.000000000000
X-C energy : (au) -11.030631380367
N-N energy : (au) 19.012380172434
---------------------------------------------------------------------------
Reaction energy (el) : (au) 0.792270103652
Reaction energy (nuc) : (au) -2.209027911782
Reaction energy (tot) : (au) -1.416757808129
---------------------------------------------------------------------------
Electronic energy : (au) -109.760261507836
Nuclear energy : (au) 16.803352260652
---------------------------------------------------------------------------
Total energy : (au) -9.295690924718e+01
: (kcal/mol) -5.833134123223e+04
: (kJ/mol) -2.440583317156e+05
: (eV) -2.529486361758e+03
===========================================================================
In response to #406.
During input parsing, now a short summary label is generated and displayed in the SCF solver table. It indicates whether an implicit solvent is being used, and if a finite external electric is applied. For example, if both are active:
A more detailed table is also printed earlier showing some key settings for the implicit solvent. For example:
To allow for the rather long names left of the
:
, some tweaks toprint_utils
were needed. AtxtBuffer
option was added that lets a developer give more room if needed (defaults to 0). Also, an option for right-aligningprint_utils::text
was added, but a left-aligned table is perhaps aesthetically more pleasing...?Should a table similar to the geometry one be printed for the cavity?