Input parsing cleaning and PhysConsts from QCelemental #404

Brakjen · 2022-03-25T09:06:39Z

Summary of changes

On the Python side

Moved the molecule validations out from python/mrchem/api.py to a separate class MoleculeValidator in python/mrchem/validators.py. I think this makes the structure and flow of the input parsing a bit clearer and readable, and cleans the responsibility of api.py to converting from user.json to program.json (with some help from helpers.py)
Added support for reading physical constants from qcelemental. Now (almost) all constants are derived directly from NIST constants collected in CODATA2018.
The constants are added to the template file via the utility script python/mrchem/update_input_parser.py, which also runs parselglossy and syncs the user ref file. The user can therefore overwrite the constant defaults in the input file for their calculation.
The constants are passed to program.json in the constants field, so that the c++ code gains access to these
Notable differences in how the constants are defined in QCElemental and (old) MRChem:

    - electron g factor        : CODATA: -2.00231930436256
                                 MRChem:  2.0023193043618

    - fine structure constant  : CODATA: 7.2973525693e-3
                                 MRChem: 7.2973525664

    - au -> wavenumbers        : CODATA: 219474.6313632
                                 MRChem: 219471.5125976648

    - au -> debye              : CODATA: 2.5417464739297717
                               : MRChem: 2.54174623105

On the c++ side

Update the interface to physical constants. A new singleton class chemistry/PhysicalConstants has been created that stores the constants from program.json in a nlohmann::json object.

ToDo

Add set of constants to c++ so that unit tests pass

…ure cleaner.

…n api.py to keep the code consistent there

…irement.

stigrj · 2022-03-25T09:55:50Z

I believe this file python/mrchem/input_parser/plumbing/init.py should not be committed. It was the one causing problems before (reason still unknown). Are you now able to run with file in source?

Brakjen · 2022-03-25T10:07:30Z

Ah, that one slipped through. Very annoying. Why has this file started causing problems?

Another thing. The ANG_2_BOHR conversion factor is present both in helpers.py and CUBEparser.py. The cube parser cannot import from helpers due to circular import errors. A simple fix solution could be to collect the constants we use during input parsing in its own file, and import from there. Perhaps ideally python and c++ should access the same set of constants (this is perhaps what you mentioned in #171)

Gabrielgerez · 2022-03-25T10:13:10Z

That li_solv has always had weird behavior, I'll check it out

stigrj · 2022-03-25T10:13:18Z

@robertodr do you know why this file suddenly started to appear?

I think one nice way to handle the constants is to let Python define them and feed them into the C++ program through the JSON input. Then we also have complete control and can tweak the parameters without recompiling, by intercepting the input parser and edit the JSON file before execution.

robertodr · 2022-03-25T10:16:36Z

@Andersmb Which version of pyparsing is installed locally? parselglossy cannot yet deal with v3 of that library, I think that's what you're seeing. I opened a PR sometime ago to fix that dev-cafe/parselglossy#109 Didn't get around to finishing it.

robertodr · 2022-03-25T10:17:12Z

The quick solution is to ignore any change in that plumbing when committing.

Brakjen · 2022-03-25T10:18:05Z

The quick solution is to ignore any change in that plumbing when committing.

I have version 3.0.7 installed...

robertodr · 2022-03-25T10:22:44Z

The quick solution is to ignore any change in that plumbing when committing.

I have version 3.0.7 installed...

Mystery understood then.

Brakjen · 2022-03-28T12:03:20Z

@Gabrielgerez I discovered why the li_solv was failing. The Molecule.cavity_coords section had a duplicated sphere

"cavity_coords": [
        {
          "center": [
            0.0,
            0.0,
            0.0
          ],
          "radius": 4.0
        },
        {
          "center": [
            0.0,
            0.0,
            0.0
          ],
          "radius": 4.0
        }
      ]

Just deleting the extra sphere lead to a passing test.

…t now passes.

codecov · 2022-03-28T12:29:23Z

Codecov Report

Merging #404 (d135280) into master (c1959c6) will decrease coverage by 0.09%.
The diff coverage is 52.11%.

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
- Coverage   68.14%   68.04%   -0.10%     
==========================================
  Files         180      182       +2     
  Lines       15170    15208      +38     
==========================================
+ Hits        10337    10349      +12     
- Misses       4833     4859      +26

Impacted Files	Coverage Δ
src/properties/Magnetizability.h	`0.00% <0.00%> (ø)`
src/qmoperators/one_electron/H_BM_dia.h	`0.00% <0.00%> (ø)`
src/qmoperators/one_electron/H_MB_dia.h	`0.00% <0.00%> (ø)`
src/qmoperators/one_electron/H_M_pso.h	`0.00% <0.00%> (ø)`
src/utils/print_utils.cpp	`76.02% <0.00%> (-12.07%)`	⬇️
src/chemistry/PhysicalConstants.cpp	`50.00% <50.00%> (ø)`
src/scf_solver/HelmholtzVector.cpp	`92.98% <50.00%> (ø)`
src/chemistry/PhysicalConstants.h	`71.42% <71.42%> (ø)`
src/analyticfunctions/HarmonicOscillatorFunction.h	`100.00% <100.00%> (ø)`
src/analyticfunctions/HydrogenFunction.cpp	`61.34% <100.00%> (ø)`
... and 11 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 c1959c6...d135280. Read the comment docs.

stigrj · 2022-04-07T14:43:51Z

I see MRCPP's pi is used once in tests/qmfunctions/orbital_vector.cpp. Can you just replace all the pi's with mrcpp::pi and remove it from MRChem's constants?

… Constants

Brakjen · 2022-04-07T15:05:10Z

Btw, I also added a PhysicalConstants::Print(int plevel) method (being called right after initialization in mrchem.cpp). I set the print threshold to zero, but I can increase it to 1.... The printout looks like this

===========================================================================
                 Physical Constants (truncated precision)
---------------------------------------------------------------------------
angstrom2bohrs                        =  1.88972612e+00
dipmom_au2debye                       =  2.54174647e+00
electron_g_factor                     = -2.00231930e+00
fine_structure_constant               =  7.29735257e-03
hartree2ev                            =  2.72113862e+01
hartree2kcalmol                       =  6.27509474e+02
hartree2kjmol                         =  2.62549964e+03
hartree2simagnetizability             =  7.89451185e+01
hartree2wavenumbers                   =  2.19474631e+05
light_speed                           =  1.37035999e+02
===========================================================================

… to pi.

…e constants. Also made the spacing adaptive to the constant names.

stigrj · 2022-04-22T06:33:59Z

python/mrchem/helpers.py

+            "light_speed": user_dict["Constants"]["light_speed"],
+            "derivative": user_dict["Derivatives"]["zora"],


These are not used in mrchem.x, light_speed is fetched directly form PhysicalConstants::get("ligth_speed") and derivative is taken as the MomentumOperator in the kinetic energy. So just remove these two parameters.

stigrj · 2022-04-22T07:04:12Z

python/template.yml

+      First boundary vector for plot.
+  - name: B
+    type: List[float]
+    default: [0.0, 1.0, 0.0]
+    predicates:
+    - len(value) == 3
+    docstring: |
+      Second boundary vector for plot.
+  - name: C
+    type: List[float]
+    default: [0.0, 0.0, 1.0]
+    predicates:
+    - len(value) == 3
+    docstring: |
+      Third boundary vector for plot.
+- name: MPI
+  docstring: |
+    Define MPI related parameters.
+  keywords:
+  - name: numerically_exact


I see the indentation has changed in the entire template.yml file, is this intentional?

hmm, this must be an effect of the yaml->dict->yaml roundtrip with ruamel.yaml. There are some options for setting indentation - I can see if I can reproduce the original indentation scheme.

I think the following gets the old indentation (source: https://yaml.readthedocs.io/en/latest/detail.html#:~:text=yaml.indent(mapping%3D2%2C%20sequence%3D4%2C%20offset%3D2))

yaml.indent(sequence=4, mapping=2, offset=2)

Excerpt:

keywords: - name: world_prec type: float predicates: - 1.0e-10 < value < 1.0 docstring: | Overall relative precision in the calculation. - name: world_size type: int default: -1 predicates: - value <= 10 docstring: | Total size of computational domain given as 2**(``world_size``). Always cubic and symmetric around the origin. Negative value means it will be computed from the molecular geometry. - name: world_unit type: str default: bohr predicates: - value.lower() in ["bohr", "angstrom"] docstring: | Length unit for *all* coordinates given in user input. Everything will be converted to atomic units (bohr) before the main executable is launched, so the JSON input is *always* given in bohrs. - name: world_origin type: List[float] default: [0.0, 0.0, 0.0] predicates: - len(value) == 3 docstring: | Global gauge origin of the calculation. sections: - name: Precisions docstring: | Define specific precision parameters. keywords: - name: nuclear_prec type: float default: user['world_prec'] predicates: - 1.0e-10 < value < 1.0 docstring: | Precision parameter used in smoothing and projection of nuclear potential. - name: poisson_prec type: float default: user['world_prec'] predicates: - 1.0e-10 < value < 1.0 docstring: | Precision parameter used in construction of Poisson operators. - name: exchange_prec type: float default: -1.0 docstring: | Precision parameter used in construction of Exchange operators. Negative value means it will follow the dynamic precision in SCF. - name: helmholtz_prec type: float default: -1.0 docstring: | Precision parameter used in construction of Helmholtz operators. Negative value means it will follow the dynamic precision in SCF.

stigrj · 2022-04-22T07:20:19Z

src/mrchem.cpp

+    // Instantiate the physical constants singleton
+    PhysicalConstants::Initialize(con_inp);
+    int print_prec = json_inp["printer"]["print_prec"];
+    if (json_inp["printer"]["print_constants"]) PhysicalConstants::Print(print_prec);


I'm not sure about the print_prec for this output. If you want to print it (print_constants == true), then you probably want the exact value that is used? There's also plenty of space on the line here, so I suggest we just fix it to print_prec = 14 decimal places here.

stigrj · 2022-04-22T07:25:23Z

src/chemistry/PhysicalConstants.cpp

+        double val = item.value().get<double>();
+        print_utils::scalar(0, key, val, "", pprec, true);
+    }
+    mrcpp::print::separator(0, '=');


Can you add some more space between this output and the next:

mrcpp::print::separator(0, '=', 2);

stigrj · 2022-04-22T07:35:35Z

src/utils/print_utils.cpp

@@ -122,7 +122,7 @@ void print_utils::coord(int level, const std::string &txt, const mrcpp::Coord<3>
 void print_utils::scalar(int level, const std::string &txt, double val, const std::string &unit, int p, bool s) {
    if (p < 0) p = Printer::getPrecision();
    int w0 = Printer::getWidth() - 2;
-    int w1 = w0 * 2 / 9;
+    int w1 = w0 * 2 / 10;


This misaligned some of the :'s in the output, but with the old value the name hartree2simagnetizability becomes too long... Perhaps abbreviate magnetizability?

stigrj · 2022-04-22T07:52:37Z

python/mrchem/update_input_parser.py

+
+
+from pathlib import Path
+from ruamel.yaml import YAML


ruamel.yaml not listed in dev-packages

…ical constants. Removed print precision option, now print constants exactly as given.

Brakjen · 2022-04-25T13:32:02Z

I think all the suggested edits have been made.

stigrj

I think this is good to go now. I'm going to squash merge due to the large number of commits

Anders Brakestad and others added 9 commits March 24, 2022 19:21

Added predicate for ensuring positive spin multiplicities

7c3b8e3

Moved the molecule validation out of the api file, to make the struct…

4df67d2

…ure cleaner.

Simplified write_molecule function

b9d3b1d

Removed method for writing to program syntax. might as well do this i…

83bad78

…n api.py to keep the code consistent there

Removed import of weird package that was sneaked in somehow

9eef63c

Cleanup and doc

73902ed

Not use walrus operator to avoid having to change python version requ…

18843e7

…irement.

fix comment typo

1605865

Removed unnecessary todo

a038d83

Brakjen requested review from stigrj and Gabrielgerez March 25, 2022 09:06

Brakjen added the WIP Work in progress label Mar 25, 2022

Brakjen marked this pull request as draft March 25, 2022 10:25

Anders Brakestad added 2 commits March 25, 2022 11:59

Removed __init__ file in input parsing plumbing

20a4217

Cleaned up the imports, moved constants to separate file temporarily to

7960907

Brakjen force-pushed the molecule-class-cleanup-input-parser branch from 0aeaeb5 to 7960907 Compare March 28, 2022 07:05

Converted test input to json

d073311

Removed duplicated cavity sphere in ref input and reran test. The tes…

005c09a

…t now passes.

Added short docstring

96a0527

Brakjen marked this pull request as ready for review March 28, 2022 13:09

Anders Brakestad added 2 commits April 7, 2022 16:49

Added a print function to PhysicalConstants. Prints at printlevel 0.

e67d732

Bug fixes related to the light speed keyword being moved from ZORA to…

19bec5d

… Constants

Removed pi from mrchem constants. Now mrcpp::pi is used for all calls…

b70a07a

… to pi.

Brakjen marked this pull request as ready for review April 7, 2022 15:11

Added constants section to input schema

ecc035a

Brakjen mentioned this pull request Apr 8, 2022

Bring the schemas à jour #405

Merged

Anders Brakestad and others added 7 commits April 8, 2022 13:53

Made constants being printed at printlevel 1 and above

acf8960

Added keyword to printer so that the user can choose when to print th…

20f0307

…e constants. Also made the spacing adaptive to the constant names.

Merge branch 'master' into molecule-class-cleanup-input-parser

13ef30f

Fix error from conflic resolution

3142afe

Merge branch 'master' into molecule-class-cleanup-input-parser

f800d3f

Minor cleanups

c7bb479

Increased distance to colon to allow for printing longer strings

52ff645

stigrj requested changes Apr 22, 2022

View reviewed changes

Anders Brakestad added 5 commits April 25, 2022 15:07

Ensure the indentation scheme in the input template.

1cf402f

Added json print helper method to print_utils. Use this to print phys…

484da12

…ical constants. Removed print precision option, now print constants exactly as given.

Undo modification to print widths.

69158fe

Removed unused parameters.

0fe4162

Added ruamel.yaml to dev packages

6c7e724

Brakjen mentioned this pull request Apr 26, 2022

Print information about the external environments #408

Merged

Anders Brakestad and others added 3 commits April 26, 2022 12:09

Updated json printer to newest version.

c0da948

Fix colon shift

e31df4f

Add cavity input section only when used

8ae0fc7

stigrj approved these changes Apr 27, 2022

View reviewed changes

Update input schema with cavity changes

d135280

stigrj merged commit 82ee2b5 into MRChemSoft:master Apr 27, 2022

Brakjen deleted the molecule-class-cleanup-input-parser branch May 19, 2022 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input parsing cleaning and PhysConsts from QCelemental #404

Input parsing cleaning and PhysConsts from QCelemental #404

Brakjen commented Mar 25, 2022 •

edited

Loading

stigrj commented Mar 25, 2022

Brakjen commented Mar 25, 2022

Gabrielgerez commented Mar 25, 2022

stigrj commented Mar 25, 2022

robertodr commented Mar 25, 2022

robertodr commented Mar 25, 2022

Brakjen commented Mar 25, 2022

robertodr commented Mar 25, 2022

Brakjen commented Mar 28, 2022

codecov bot commented Mar 28, 2022 •

edited

Loading

stigrj commented Apr 7, 2022

Brakjen commented Apr 7, 2022

stigrj Apr 22, 2022

stigrj Apr 22, 2022

Brakjen Apr 25, 2022

Brakjen Apr 25, 2022

stigrj Apr 22, 2022

stigrj Apr 22, 2022

stigrj Apr 22, 2022

stigrj Apr 22, 2022

Brakjen commented Apr 25, 2022

stigrj left a comment

		"light_speed": user_dict["Constants"]["light_speed"],
		"derivative": user_dict["Derivatives"]["zora"],

Input parsing cleaning and PhysConsts from QCelemental #404

Input parsing cleaning and PhysConsts from QCelemental #404

Conversation

Brakjen commented Mar 25, 2022 • edited Loading

Summary of changes

On the Python side

On the c++ side

ToDo

stigrj commented Mar 25, 2022

Brakjen commented Mar 25, 2022

Gabrielgerez commented Mar 25, 2022

stigrj commented Mar 25, 2022

robertodr commented Mar 25, 2022

robertodr commented Mar 25, 2022

Brakjen commented Mar 25, 2022

robertodr commented Mar 25, 2022

Brakjen commented Mar 28, 2022

codecov bot commented Mar 28, 2022 • edited Loading

Codecov Report

stigrj commented Apr 7, 2022

Brakjen commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brakjen commented Apr 25, 2022

stigrj left a comment

Choose a reason for hiding this comment

Brakjen commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 28, 2022 •

edited

Loading