Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ENVIRON namelists #703

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a991ec6
Added boundary and electrostatic namelists for ENVIRON
Jun 5, 2021
d2691b8
Added example environ calculation for testing purposes
Jun 5, 2021
a134570
Merge branch 'aiidateam:develop' into develop
sudarshanv01 Jul 11, 2021
d753459
Added test for generating Environ namelist
Jul 11, 2021
726b355
Added some documentation about how to run Environ and some enabled pa…
Jul 11, 2021
64161bc
Fixed typo in parsing of Environ
Jul 15, 2021
12be851
Merge branch 'develop' into develop
mbercx Jul 22, 2021
9fc3a1a
Added tests to pw parser and calculation and added log to exception o…
Jul 23, 2021
b5d2859
Merge branch 'develop' of git+ssh://github.com/sudarshanv01/aiida-qua…
Jul 23, 2021
29e0574
Merge branch 'develop' of git+ssh://github.com/sudarshanv01/aiida-qua…
Jul 23, 2021
894b95a
Merge branch 'develop' of git+ssh://github.com/sudarshanv01/aiida-qua…
Jul 23, 2021
e38ddf1
Merge branch 'develop' of github.com:sudarshanv01/aiida-quantumespres…
Aug 7, 2021
8f0bb86
Make environ tests work in both calculations and parser
Aug 7, 2021
1f6dd1b
Changes from pre-commit to Environ test
Aug 7, 2021
db7d722
Changes from pre-commit to Environ test
Aug 7, 2021
f657f45
Added parser test, calculation files for Environ
Nov 21, 2021
9ef55c5
Added parser test, calculation files for Environ
Nov 21, 2021
4168fee
Added parser test, calculation files for Environ
Nov 21, 2021
5009842
Run environ calculation parser only if it is an environ calculation.
Nov 22, 2021
3d13abb
Merge environ namelists test into one
Nov 22, 2021
90aae90
Fixed is_environ tag
Nov 22, 2021
acb2d0a
Update docs/source/user_guide/get_started/examples/pw_tutorial.rst
sudarshanv01 Nov 23, 2021
c8016b8
Update tests/conftest.py
sudarshanv01 Nov 23, 2021
fe50cfa
Update tests/conftest.py
sudarshanv01 Nov 23, 2021
5764e4f
Update tests/parsers/test_pw.py
sudarshanv01 Nov 23, 2021
ed7e70c
Update tests/calculations/test_pw.py
sudarshanv01 Nov 23, 2021
94d1e85
Update tests/parsers/test_pw.py
sudarshanv01 Nov 23, 2021
2e5bc1b
Removed Pt structure from conftest
Nov 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions aiida_quantumespresso/calculations/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,11 @@ def prepare_for_submission(self, folder):
if environ_namelist is not None:
if not isinstance(environ_namelist, dict):
raise exceptions.InputValidationError('ENVIRON namelist should be specified as a dictionary')

## Check for BOUNDARY and ELECTROSTATIC Keys as well
boundary_namelist = settings.pop('BOUNDARY', None)
electrostatic_namelist = settings.pop('ELECTROSTATIC', None)

# We first add the environ flag to the command-line options (if not already present)
try:
if '-environ' not in settings['CMDLINE']:
Expand All @@ -255,6 +260,18 @@ def prepare_for_submission(self, folder):
handle.write(convert_input_to_namelist_entry(key, value, mapping=mapping_species))
handle.write('/\n')

if boundary_namelist is not None:
handle.write('&BOUNDARY\n')
for key, value in sorted(boundary_namelist.items()):
handle.write(convert_input_to_namelist_entry(key, value, mapping=mapping_species))
handle.write('/\n')

if electrostatic_namelist is not None:
handle.write('&ELECTROSTATIC\n')
for key, value in sorted(electrostatic_namelist.items()):
handle.write(convert_input_to_namelist_entry(key, value, mapping=mapping_species))
handle.write('/\n')

# Check for the deprecated 'ALSO_BANDS' setting and if present fire a deprecation log message
also_bands = settings.pop('ALSO_BANDS', None)
if also_bands:
Expand Down
9 changes: 9 additions & 0 deletions aiida_quantumespresso/parsers/parse_raw/pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,15 @@ def parse_stdout(stdout, input_parameters, parser_options=None, parsed_xml=None)
parsed_data['fermi_energy' + units_suffix] = default_energy_units
except Exception:
logs.warning.append('Error while parsing Fermi energy from the output file.')
elif 'Gaussian-smeared nuclei' in line:
try:
value_potential_shift = float(line.split()[-2])
unit_potential_shift = line.split()[-1]
parsed_data['environ_potential_shift'] = value_potential_shift
parsed_data['environ_unit_potential_shift'] = unit_potential_shift
except Exception:
logs.warning.append('Not an Environ calculation with an open boundary condition.')
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a log here as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding: is the string "Gaussian-smeared nuclei" specific to ENVIRON calculations? Because if that is the case, the warning message in the except case, doesn't make a lot of sense, does it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I guess it is specific to ENVIRON calculations, right now it says Not an Environ calculation with an open boundary condition, but this might show up for all calculations, which might be a bit annoying? Is there a better way to have a log message here without it popping up every time - and only for ENVIRON calculations?

Copy link
Contributor

@sphuber sphuber Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, just add a conditional to the elif 'Gaussian-smeared nuclei' in line to only run when it is an ENVIRON calculation. How to determine whether we actually are parsing an ENVIRON question depends. Probably you could parse the stdout for some flag, but this is really fragile. If the XML doesn't contain some switch that we can rely on (which would be the best and most robust solution) you can use the input parameters. If I understand correctly, if you want to run an ENVIRON calculation, you have to add a particular key to the parameters input node. You can check for that at the beginning of the parser. Something like

is_environ = 'ENVIRON' in self.inputs.settings.get_dict()

and then the parsing line would become

elif is_environ and 'Gaussian-smeared nuclei' in line:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, absolutely - just put this change in. In the current form I would need to pass an extra argument of settings to the parse_raw function. I could also just send is_environ as an argument, but maybe it is more complete to pass the settings dictionary, just like parameters?


elif 'Forces acting on atoms' in line:
try:
Expand Down
146 changes: 146 additions & 0 deletions docs/source/user_guide/get_started/examples/environ_calculation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
# -*- coding: utf-8 -*-

from aiida import orm
from aiida.engine import run, submit
import numpy as np
from ase import Atoms
from ase import units
"""
Calculation that recreates the environ calculation of
CO on Pt(111) with a unit charge

More details about the solvation models and potential corrections
can be found here:

O. Andreussi, I. Dabo and N. Marzari, J. Chem. Phys. 136,064102 (2012).
I. Dabo et al. Phys.Rev. B 77, 115139 (2008)

This system is taken directly from Example04 in the
examples folder from Environ
"""


def calculator():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you make this and the environ_calculator a function if all it does is return a static dictionary? I think it might be simpler if to literally just include the dictionary in the main method where you assign it directly to the building

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup good point, has been corrected.

"""
This is the SCF calculator, which is identical
and unchanged from what one would do in the case
of a standard SCF calculation - nothing environ
related. Remember to add in the tot_charge here
"""
param_dict = {
'CONTROL': {
'calculation': 'scf',
'tprnfor': True,
},
'SYSTEM': {
'ecutwfc': 35,
'ecutrho': 280,
'occupations': 'smearing',
'smearing': 'cold',
'degauss': 0.03,
'tot_charge': 1,
},
'ELECTRONS': {
'conv_thr': 5e-9,
'electron_maxstep': 200,
'mixing_beta': 0.2,
'mixing_ndim': 15,
'diagonalization': 'david',
},
}

return param_dict


def environ_calculator():
"""
Environ parameters
"""
param_dict = {
'ENVIRON': {
'verbose': 0,
'environ_thr': 1.0,
'environ_type': 'input',
'env_static_permittivity': 1,
'env_surface_tension': 0.0,
'env_pressure': 0.0,
'env_electrostatic': True,
},
'BOUNDARY': {
'solvent_mode': 'full',
},
'ELECTROSTATIC': {
'pbc_correction': 'parabolic',
'pbc_dim': 2,
'pbc_axis': 3,
'tol': 5e-13,
}
}
return param_dict


def runner(code, structure):

## Base calculation for testing
PwBaseWorkChain = WorkflowFactory('quantumespresso.pw.base')
builder = PwBaseWorkChain.get_builder()
builder.pw.structure = structure

builder.metadata.label = 'Single point environ'
builder.metadata.description = 'Testing calculation with environ for Pt(111) + CO'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hardcodes the structure even though the structure is mutable through the argument. Not a huge deal since it is an example, but still a bit weird.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, also corrected.


KpointsData = DataFactory('array.kpoints')
kpoints = KpointsData()
kpoints.set_kpoints_mesh([1, 1, 1]) # Not physical just a test
builder.kpoints = kpoints

family = load_group('SSSP/1.1/PBE/efficiency')
builder.pw.pseudos = family.get_pseudos(structure=structure)

calculation = calculator()
environ_calculation = environ_calculator()
## these are the main cube files that could potentially be parsed
## if the verbosity is set to 2 or higher
# environ_calculation['additional_retrieve_list'] = ['epsilon.cube', \
# 'vreference.cube', 'velectrostatic.cube', 'vsoftcavity.cube', \
# 'electrons.cube', 'charges.cube', 'smeared_ions.cube']

builder.pw.parameters = orm.Dict(dict=calculation)
builder.pw.settings = orm.Dict(dict=environ_calculation)
builder.pw.metadata.options.resources = {'num_machines': 1}
builder.pw.metadata.options.max_wallclock_seconds = 5 * 60
builder.pw.code = code

calculation = submit(builder)


if __name__ == '__main__':
code = load_code('pw_6-7@juwels')

StructureData = DataFactory('structure')
## these are the original coordinates for the Pt-CO system
positions = [
[5.335084148, 4.646723426, 12.901029877],
[5.335009643, 4.619623254, 15.079854269],
[8.061327071, 0.098057998, 8.992142901],
[2.608989366, 0.098058283, 8.992140585],
[0.000036609, 4.720846294, 8.968756935],
[5.335159557, 4.721612729, 9.380196435],
[0.000041121, 7.802951963, 4.604626508],
[5.335161233, 7.697749113, 4.753489408],
[2.697860636, 3.152173889, 4.688412329],
[7.972463687, 3.152174491, 4.688415209],
]

## setting up the system with ASE
## notice the units that are being used
atoms = Atoms('COPt8')
atoms.set_positions(np.array(positions) * units.Bohr)
atoms.set_pbc([True, True, True])
a = 10.6881 * units.Bohr
b = 0.866025 * a * units.Bohr
c = 3.95422 * a * units.Bohr
atoms.set_cell([a, b, c])
structure = StructureData(ase=atoms)

runner(code=code, structure=structure)
26 changes: 26 additions & 0 deletions docs/source/user_guide/get_started/examples/pw_tutorial.rst
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,32 @@ More options are available, and can be explored by expanding
``builder.metadata.options.`` + ``TAB``.


Environ calculations
--------------------

Environ calculations can be run by supplying the required namelists to ``builder.settings``. For example, to run a calculation with a continuum dielectric of water above a 2D slab with the surface normal in the z-direction create the following dictionary.

::

environ_param_dict = {
'ENVIRON':{
'environ_type': 'input',
'env_static_permittivity': 78.36,
'env_electrostatic': True,
},
'BOUNDARY':{
'solvent_mode':'full',
},
'ELECTROSTATIC':{
'pbc_correction':'parabolic',
'pbc_dim': 2,
'pbc_axis': 3,
}
}

This dictionary can be passed on to the builder as ::

builder.pw.settings = orm.Dict(dict=environ_param_dict)
sudarshanv01 marked this conversation as resolved.
Show resolved Hide resolved


Launching the calculation
Expand Down
30 changes: 30 additions & 0 deletions tests/calculations/test_pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,3 +250,33 @@ def test_pw_parallelization_duplicate_cmdline_flag(fixture_sandbox, generate_cal
with pytest.raises(InputValidationError) as exc:
generate_calc_job(fixture_sandbox, entry_point_name, inputs)
assert 'Conflicting' in str(exc.value)


def test_environ_namelists(fixture_sandbox, generate_calc_job, generate_inputs_pw, file_regression):
"""Test that Environ namelists are created."""
entry_point_name = 'quantumespresso.pw'

inputs = generate_inputs_pw()
inputs['settings'] = orm.Dict(
dict={
'ENVIRON': {
'electrolyte_linearized': True,
'environ_type': 'input',
},
'BOUNDARY': {
'solvent_mode': 'electronic',
'electrolyte_mode': 'electronic',
},
'ELECTROSTATIC': {
'pbc_correction': 'parabolic',
}
},
)
generate_calc_job(fixture_sandbox, entry_point_name, inputs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not actually yet testing that the namelists are (properly) generates. The generate_calc_job just creates the CalcJob and calls the prepare_for_submission method. You still need to check that the input file that was generated is correct. To see how to do this, you can look in this same file at the test test_pw_default all the way at the top:

    with fixture_sandbox.open('aiida.in') as handle:
        input_written = handle.read()
        file_regression.check(input_written, encoding='utf-8', extension='.in')

The file_regression fixture will compare the input file that was written with a reference file. If that file doesn't yet exist (i.e. before you have ever run this test) it will automatically create it. If you rerun the tests a second time, it will work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sphuber - thanks a lot for the comments! I have a few questions about how these tests work. I have added in file_regression in the most recent commit, but I have not really set up a corresponding input file that it should compare to, and I missing how it automatically creates it.

Regarding the parser, I modeled the Environ test after the test_pw_default test and added in some checks after running an Environ calculation. A very basic question - how does this work in practice, is a calculation actually run - in which case it might break because Environ is a plugin that doesn't ship with the code I believe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just add the file_regression call in the test, the comparison will be automatically created once you run the tests. If you then run the tests a second time, the comparison file is there and the test should therefore pass. If you ever change anything in the test that would imply that the comparison file has to be updated, you don't have to do this manually either. If you run the tests and they fail because the comparison file is outdated, you can update it by rerunning the tests with the flag --force-regen and it will update all comparison files that are used and whose test fails.

A very basic question - how does this work in practice, is a calculation actually run - in which case it might break because Environ is a plugin that doesn't ship with the code I believe.

No, it doesn't run the calculation. I set up the framework for these parser tests that it simply mocks an already completed calculation with outputs taking from the test directory. Each test has its own folder that corresponds to the name of the test. So all you have to do is add a folder for your test with the output files of a run you did manually with the environ module enabled.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! So I think I understand now how the tests for the calculations work (and have updated the latest commit with the right files). The same thing seems to work for the parser, but I am not sure I am testing this properly. All inputs to Environ would be passed through the settings, so I am not sure if this information is included in the yml that is generated.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests now look fine 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!


with fixture_sandbox.open('aiida.in') as handle:
input_written = handle.read()

# Checks on the files written to the sandbox folder as raw input
assert sorted(fixture_sandbox.get_content_list()) == sorted(['aiida.in', 'pseudo', 'out', 'environ.in'])
file_regression.check(input_written, encoding='utf-8', extension='.in')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the main input file aiida.in (as you are doing here) is not bad, but as you can see from the generated comparison file, it doesn't actually include any of the environ settings, which are written to environ.in instead. So really you are still not really testing the environ specific functionality in this test. So you should change checking the content of the aiida.in for the environ.in file instead.

Maybe it could even be argued that it would be useful to check both files because theoretically the environ related code could break the code that writes the main input file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sphuber - apologies that this reply is so late, this pr seems to have slipped through somehow. Just made some changes. I have now added two tests now as you suggested, one for the aiida.in file called test_environ_pw_namelists and the second for environ.in files called test_environ_namelists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need two different tests for this? The tests are identical, you are just checking a different file in each. You can merge these tests and check both files in one with something like this:

file_regression.check(input_written, encoding='utf-8', extension='.aiida.in')
file_regression.check(input_written, encoding='utf-8', extension='.environ.in')

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup I do not need two tests for this as you mentioned. Has been combined into one as you said!

27 changes: 27 additions & 0 deletions tests/calculations/test_pw/test_environ_namelists.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
&CONTROL
calculation = 'scf'
outdir = './out/'
prefix = 'aiida'
pseudo_dir = './pseudo/'
verbosity = 'high'
/
&SYSTEM
ecutrho = 2.4000000000d+02
ecutwfc = 3.0000000000d+01
ibrav = 0
nat = 2
ntyp = 1
/
&ELECTRONS
/
ATOMIC_SPECIES
Si 28.0855 Si.upf
ATOMIC_POSITIONS angstrom
Si 0.0000000000 0.0000000000 0.0000000000
Si 1.3575000000 1.3575000000 1.3575000000
K_POINTS automatic
2 2 2 0 0 0
CELL_PARAMETERS angstrom
2.7150000000 2.7150000000 0.0000000000
2.7150000000 0.0000000000 2.7150000000
0.0000000000 2.7150000000 2.7150000000
32 changes: 32 additions & 0 deletions tests/parsers/test_pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -907,3 +907,35 @@ def test_pw_vcrelax_failed_not_converged_nstep(
assert 'output_parameters' in results
assert 'output_structure' in results
assert 'output_trajectory' in results


def test_environ(
fixture_localhost,
generate_calc_job_node,
generate_parser,
generate_inputs,
data_regression,
):
"""Test a simple Environ calculation."""
name = 'default'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are almost there with creating a parser test. This name variable here, actually corresponds to a folder in the tests/parsers/fixtures/pw folder. You will find the default folder there. It actually contains output files that were generated by a PwCalculation, i.e. the aiida.out and data-file.xml. Through the generate_calc_job_node function called on line 924, a CalcJobNode is mocked with a retrieved output node that contains these files. This is replicating what happens when you actually run a PwCalculation and the node is passed to the parser.

What you then have to do is create a new folder in that fixtures base folder, let's call it default_environ and put the aiida.out and data-file.xml in there that you take from an actual PwCalculation run that included the ENVIRON namecard. Only then will you actually test the parser for output files that were produced by an environ run. In the current state, you are simply testing exactly the same as the test_default test, which tests the outputs for a simple scf calculation with pw.x. I hope this is clear. If not, feel to ask for more clarification.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup all makes sense - thanks! Let me just describe what I did here. I added a new structure with structure_id=platinum to the conftest.py in order to test the different environ outputs as they won't work for the two example cases for Si or water (I need a 2D slab to check for the right flags). I then ran an pw calculation with QE v.6.7 and stored the aiida.out and the data-file.xml file in fixtures/default_environ. So far so good, except the auto-generated test_environ.yml file doesn't seem to have the right specifications (it reports Si and not Pt). Moreover the k-points do not seem to be parsed properly (commented out currently). I can also bypass this and run everything for Si, but my thinking is that it would be nice to have the test mirror that of the Environ package.

entry_point_calc_job = 'quantumespresso.pw'
entry_point_parser = 'quantumespresso.pw'
environ_settings = {'ENVIRON': {'environ_type': 'water'}}
node = generate_calc_job_node(
entry_point_calc_job, fixture_localhost, name, generate_inputs(settings=environ_settings)
)
parser = generate_parser(entry_point_parser)
results, calcfunction = parser.parse_from_node(node, store_provenance=False)

assert calcfunction.is_finished, calcfunction.exception
assert calcfunction.is_finished_ok, calcfunction.exit_message
assert not orm.Log.objects.get_logs_for(node), [log.message for log in orm.Log.objects.get_logs_for(node)]
assert 'output_kpoints' in results
assert 'output_parameters' in results
assert 'output_trajectory' in results

data_regression.check({
'output_kpoints': results['output_kpoints'].attributes,
'output_parameters': results['output_parameters'].get_dict(),
'output_trajectory': results['output_trajectory'].attributes,
})
Loading