Print information about the external environments #408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
stigrj
requested changes
Apr 22, 2022
src/environment/SCRF.cpp
Outdated
| @@ -288,4 +289,21 @@ void SCRF::updateCurrentGamma(QMFunction &gamma_np1) { | |||
| gamma_np1.free(NUMBER::Real); | |||
| } | |||
|
|
|||
| void SCRF::printParameters() { | |||
There was a problem hiding this comment.
We might want some information on the actual cavity as well, because I guess we can set up a calculation with just random spheres as cavity, e.i. not restricted to atomic coordinates and vdW radius. Is this right @Gabrielgerez ?
There was a problem hiding this comment.
Yes, there is a possibility to specify the cavity sphere locations in the input. If not specified, then nuclear coordinates are used.
There was a problem hiding this comment.
How does this look?
===========================================================================
Solvation Cavity
---------------------------------------------------------------------------
Cavity width : 0.500000
---------------------------------------------------------------------------
N Radius : x y z
---------------------------------------------------------------------------
0 4.000000 : 0.000000 0.000000 0.000000
1 4.000000 : 0.000000 0.000000 0.000000
===========================================================================produced from
/** @brief Pretty output of solvation cavity spheres */
void Molecule::printCavity() {
// Collect relevant quantities
Cavity cavity = getCavity();
std::vector<mrcpp::Coord<3>> coords = cavity.getCoordinates();
std::vector<double> radii = cavity.getRadii();
// Set widths
auto w0 = Printer::getWidth() - 1;
auto w1 = 5;
auto w2 = 12;
auto w3 = 2 * w0 / 9;
auto w4 = w0 - w1 - w2 - 3 * w3;
// Build table column headers
std::stringstream o_head;
o_head << std::setw(w1) << "N";
o_head << std::setw(w2) << "Radius";
o_head << std::string(w4 - 1, ' ') << ':';
o_head << std::setw(w3) << "x";
o_head << std::setw(w3) << "y";
o_head << std::setw(w3) << "z";
// Print
mrcpp::print::header(0, "Solvation Cavity");
print_utils::scalar(0, "Cavity width", cavity.getWidth(), "", 0);
mrcpp::print::separator(0, '-');
println(0, o_head.str());
mrcpp::print::separator(0, '-');
for (int i = 0; i < coords.size(); i++) {
mrcpp::Coord<3> coord = coords[i];
double r = radii[i];
double x = coord[0];
double y = coord[1];
double z = coord[2];
std::stringstream o_coord;
o_coord << std::setw(w1) << i;
o_coord << std::setw(w2) << std::setprecision(6) << std::fixed << r;
o_coord << std::string(w4 - 1, ' ') << ':';
o_coord << std::setw(w3) << std::setprecision(6) << std::fixed << x;
o_coord << std::setw(w3) << std::setprecision(6) << std::fixed << y;
o_coord << std::setw(w3) << std::setprecision(6) << std::fixed << z;
println(0, o_coord.str());
}
mrcpp::print::separator(0, '=', 2);
}Should some of this be abstracted to print_utils::cavity? I see that's the way it is done when printing the molecular geometry.
There was a problem hiding this comment.
Looks good!
You mean like print_utils::coord()? No I think it is fine like this, coord is a bit more general and useful to extract.
There was a problem hiding this comment.
Okay, then I'll leave it as it is 👍
src/utils/print_utils.cpp
Outdated
Comment on lines
94
to
96
| void print_utils::text(int level, const std::string &txt, const std::string &val, bool ralign, int txtBuffer) { | ||
| int w0 = Printer::getWidth() - 2; | ||
| int w1 = w0 * 2 / 9; | ||
| int w1 = w0 * 2 / 9 - txtBuffer; |
There was a problem hiding this comment.
Not sure how to properly resolve this. We should have something that looks good with the default print_width and print_precision, but that doesn't crash if we make things too narrow, or when inserting too long strings. So either we truncate the text string to always make it fit, or we always print the full string, even if it becomes unaligned with the rest?
There was a problem hiding this comment.
As a quick workaround, I thought perhaps of adding a print_utils::json method, that takes a json object, determines the longest name of the collection, and adjusts the colon placement accordingly. Could also have an option for right-aligning it. Something like this:
void print_utils::json(int level, const nlohmann::json &j, bool ralign) {
// Determine longest name
int w = 0;
for (const auto &item : j.items()) {
if (item.key().size() > w) w = item.key().size();
}
// Print
for (const auto &item : j.items()) {
std::string key = item.key();
std::stringstream o_val;
o_val << item.value();
std::string val = o_val.str();
// Remove quotes from val and print
val.erase(std::remove(val.begin(), val.end(), '\"'), val.end());
// If right-align, determine how much to shift the vals
int shift = (ralign) ? Printer::getWidth() - w - val.size() - 3 : 0;
// Avoid runtime errors due to negative shifts caused by very long names
if (shift < 0) shift = 0;
std::printf("%-*s%-s%-s%-s\n", w, key.c_str(), " : ", std::string(shift, ' ').c_str(), val.c_str());
}
}and using it like this
void SCRF::printParameters() {
nlohmann::json data = {
{"Dielectric constant (inside)", epsilon.getEpsIn()},
{"Dielectric constant (outside)", epsilon.getEpsOut()},
{"Max. number of micro-iterations", max_iter},
{"Accelerate with KAIN", (accelerate_Vr) ? "true" : "false"},
{"Algorithm", algorithm},
{"Density type", density_type},
{"Convergence criterion", convergence_criterion},
};
mrcpp::print::header(0, "Self-Consistent Reaction Field");
print_utils::json(0, data, true);
mrcpp::print::separator(0, '=', 2);
}which produces
===========================================================================
Self-Consistent Reaction Field
---------------------------------------------------------------------------
Accelerate with KAIN : true
Algorithm : scrf
Convergence criterion : dynamic
Density type : total
Dielectric constant (inside) : 1.0
Dielectric constant (outside) : 2.0
Max. number of micro-iterations : 100
===========================================================================For too long names (negative shift), just set shift = 0 to avoid runtime errors
===========================================================================
Self-Consistent Reaction Field
---------------------------------------------------------------------------
A very very very very very very very very very very very very very very very very very long name : infty
Accelerate with KAIN : true
Algorithm : scrf
Convergence criterion : dynamic
Density type : total
Dielectric constant (inside) : 1.0
Dielectric constant (outside) : 2.0
Max. number of micro-iterations : 100
===========================================================================There was a problem hiding this comment.
Two more nitpicky things on the json printer:
- Each line should start and end with a whitespace
- I would like the colons on all the different sections in the output to line up (unless you provide a
very very very longname), so the string length should be minimumw1
int w0 = Printer::getWidth() - 2;
int w1 = w0 * 2 / 9;
There was a problem hiding this comment.
Okay, the method should adhere to these criteria now :)
Left- and right-aligned sample sections compared to a Molecule section:
===========================================================================
Molecule
---------------------------------------------------------------------------
Charge : 1
Multiplicity : 1
---------------------------------------------------------------------------
N Atom : x y z
---------------------------------------------------------------------------
0 Li : 0.000000 0.000000 0.000000
---------------------------------------------------------------------------
Center of mass : 0.000000 0.000000 0.000000
===========================================================================
---------------------------------------------------------------------------
k s sdf : 1.0
kjsdhf kjsf : 1.0
ks jdfkjs : 1.0
---------------------------------------------------------------------------
k s sdf : 1.0
kjsdhf kjsf : 1.0
ks jdfkjs : 1.0
---------------------------------------------------------------------------
jd jfdhfj dhf : 1.0
jdj f djf hdjhfj dh fjd fjd djfhfj : 1.0
jh djfh jdfhjdh fjd fhjdh fjd : 1.0
---------------------------------------------------------------------------
jd jfdhfj dhf : 1.0
jdj f djf hdjhfj dh fjd fjd djfhfj : 1.0
jh djfh jdfhjdh fjd fhjdh fjd : 1.0
---------------------------------------------------------------------------
jd jfdhfj dhf : 1.0
jdj f djf hdjhfj dh fjd fjd hdfj dhf jdhf jdhf jdfhjd fhjdhfjdhf jd fdjfhfj : 1.0
jh djfh jdfhjdh fjd fhjdh fjd : 1.0
---------------------------------------------------------------------------
jd jfdhfj dhf : 1.0
jdj f djf hdjhfj dh fjd fjd hdfj dhf jdhf jdhf jdfhjd fhjdhfjdhf jd fdjfhfj : 1.0
jh djfh jdfhjdh fjd fhjdh fjd : 1.0
---------------------------------------------------------------------------
|
Where is the appropriate place to print information about SCRF and the cavity? Now I am printing in if (json_fock.contains("reaction_operator")) {
// preparing Reaction Operator
(...)
mol.printCavity();
V_R->getHelper()->printParameters();
}
|
|
|
|
Ok, we need to fix this, there should not be any |
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 |
|
We should also fix some of the solvent related printouts:
|
|
Now 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 |
Before I mess everything up: Is this what I should do? First do a git rebase -i 8ae9f19dand change And then do a git rebase upstream/masterand resolve conflicts? And then a git push --force? |
|
Sounds about right, starting with a |
Brakjen
force-pushed
the
solvent-printouts
branch
3 times, most recently
from
b3a13b0
to
c41717c
Compare
|
I'll give it a 10% chance that I did the rebase correctly... |
|
Looks like the |
|
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 ***************************************************************************
*** ***
*** 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 🙂 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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
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 👍 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 Perhaps it's the |
I have moved the printing to But when printing these just after construction, which would be in Output file: Parameters printed in `driver::build_fock_operator`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 If I move the SCRF and cavity printouts to Output file: Parameters printed in `GroundStateSolver::optimize`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 @Gabrielgerez why did we disable solvent effects on the I will add some of these changes to this branch tomorrow. |
…to align with other print sections if possible.
…tCavity statement.
…run if solvent is requested.
stigrj
approved these changes
Jul 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_utilswere needed. AtxtBufferoption was added that lets a developer give more room if needed (defaults to 0). Also, an option for right-aligningprint_utils::textwas added, but a left-aligned table is perhaps aesthetically more pleasing...?Should a table similar to the geometry one be printed for the cavity?