-
Notifications
You must be signed in to change notification settings - Fork 82
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
base: main
Are you sure you want to change the base?
Changes from 21 commits
a991ec6
d2691b8
a134570
d753459
726b355
64161bc
12be851
9fc3a1a
b5d2859
29e0574
894b95a
e38ddf1
8f0bb86
1f6dd1b
db7d722
f657f45
9ef55c5
4168fee
5009842
3d13abb
90aae90
acb2d0a
c8016b8
fe50cfa
5764e4f
ed7e70c
94d1e85
2e5bc1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -267,7 +267,7 @@ def detect_important_message(logs, line): | |
logs.warning.append(message) | ||
|
||
|
||
def parse_stdout(stdout, input_parameters, parser_options=None, parsed_xml=None): | ||
def parse_stdout(stdout, input_parameters, parser_options=None, parsed_xml=None, settings={}): | ||
"""Parses the stdout content of a Quantum ESPRESSO `pw.x` calculation. | ||
|
||
:param stdout: the stdout content as a string | ||
|
@@ -306,6 +306,9 @@ def parse_stdout(stdout, input_parameters, parser_options=None, parsed_xml=None) | |
# Determine whether the input switched on an electric field | ||
lelfield = input_parameters.get('CONTROL', {}).get('lelfield', False) | ||
|
||
# Determine if an ENVIRON calculation run | ||
is_environ = 'ENVIRON' in settings | ||
|
||
# Find some useful quantities. | ||
if not parsed_xml.get('number_of_bands', None): | ||
try: | ||
|
@@ -743,6 +746,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 is_environ and '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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a log here as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Giovanni! I have added it to the log in the most recent commit - thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, just add a conditional to the 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
elif 'Forces acting on atoms' in line: | ||
try: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
# -*- 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 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' | ||
|
||
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 = { | ||
'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', | ||
}, | ||
} | ||
|
||
environ_calculation = { | ||
'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, | ||
} | ||
} | ||
# 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('insert-code-here') | ||
|
||
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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,7 +73,7 @@ def test_pw_ibrav( | |
calc_info = generate_calc_job(fixture_sandbox, entry_point_name, inputs) | ||
|
||
cmdline_params = ['-in', 'aiida.in'] | ||
local_copy_list = [(upf.uuid, upf.filename, u'./pseudo/Si.upf')] | ||
local_copy_list = [(upf.uuid, upf.filename, './pseudo/Si.upf')] | ||
retrieve_list = ['aiida.out', './out/aiida.save/data-file-schema.xml', './out/aiida.save/data-file.xml'] | ||
retrieve_temporary_list = [['./out/aiida.save/K*[0-9]/eigenval*.xml', '.', 2]] | ||
|
||
|
@@ -250,3 +250,37 @@ 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 does not change the contents of the pw file 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
The There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Regarding the parser, I modeled the Environ test after the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you just add the
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests now look fine 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
with fixture_sandbox.open('environ.in') as handle: | ||
environ_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']) | ||
# Check the aiida.in files are written correctly | ||
sudarshanv01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
file_regression.check(input_written, encoding='utf-8', extension='.aiida.in') | ||
file_regression.check(environ_written, encoding='utf-8', extension='.environ.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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
&ENVIRON | ||
electrolyte_linearized = .true. | ||
environ_type = 'input' | ||
/ | ||
&BOUNDARY | ||
electrolyte_mode = 'electronic' | ||
solvent_mode = 'electronic' | ||
/ | ||
&ELECTROSTATIC | ||
pbc_correction = 'parabolic' | ||
/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,7 +234,7 @@ def _generate_calc_job_node( | |
from qe_tools.exceptions import ParsingError | ||
from aiida_quantumespresso.tools.pwinputparser import PwInputFile | ||
try: | ||
with open(filepath_input, 'r') as input_file: | ||
with open(filepath_input, 'r') as input_file: # pylint: disable=unspecified-encoding | ||
sudarshanv01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
parsed_input = PwInputFile(input_file.read()) | ||
except (ParsingError, FileNotFoundError): | ||
pass | ||
|
@@ -314,8 +314,21 @@ def _generate_structure(structure_id='silicon'): | |
structure.append_atom(position=[12.73464656, 16.7741411, 24.35076238], symbols='H', name='H') | ||
structure.append_atom(position=[-29.3865565, 9.51707929, -4.02515904], symbols='H', name='H') | ||
structure.append_atom(position=[1.04074437, -1.64320127, -1.27035021], symbols='O', name='O') | ||
elif structure_id == 'platinum': | ||
# Used for environ tests | ||
structure = StructureData(cell=[[10.6881, 0., 0.], [0., 9.2561, 0.], [0., 0., 42.2630]]) | ||
structure.append_atom(position=[5.335084148, 4.646723426, 12.901029877], symbols='C', name='C') | ||
structure.append_atom(position=[5.335009643, 4.619623254, 15.079854269], symbols='O', name='O') | ||
structure.append_atom(position=[8.061327071, 0.098057998, 8.992142901], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[2.608989366, 0.098058283, 8.992140585], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[0.000036609, 4.720846294, 8.968756935], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[5.335159557, 4.721612729, 9.380196435], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[0.000041121, 7.802951963, 4.604626508], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[5.335161233, 7.697749113, 4.753489408], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[2.697860636, 3.152173889, 4.688412329], symbols='Pt', name='Pt') | ||
structure.append_atom(position=[7.972463687, 3.152174491, 4.688415209], symbols='Pt', name='Pt') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really need a different structure for the tests? Usually you are just mocking a structure and the test shouldn't really care about what actual structure is used. I want to prevent we start adding a bunch of structures that are not necessary at all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very fair - as long as there is some vacuum somewhere. Maybe the structure of water could be used instead, would that work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What part of the code will fail if there is no vacuum? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it would necessarily fail, I initially thought it would be nice to test the pbc corrections schemes, as we have them in the parser now, though this is relatively minor. If we are allowed to use a different structure from the one that was used for the DFT calculation, then that would be checked anyway. I have now removed the alternate structure (but allowed for the the option of |
||
else: | ||
raise KeyError('Unknown structure_id=\'{}\''.format(structure_id)) | ||
raise KeyError('Unknown structure_id=\'{}\''.format(structure_id)) # pylint: disable=consider-using-f-string | ||
sudarshanv01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return structure | ||
|
||
return _generate_structure | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that using mutable objects (as a
dict
is) for function defaults is dangerous. The reason is that the default can actually change if it is mutated. Rather, you should definesettings=None
and then in the function body you callsettings = settings or {}
which will assign the empty dict if it isNone
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that makes sense, good to know - thanks!