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 20 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
14 changes: 13 additions & 1 deletion aiida_quantumespresso/parsers/parse_raw/pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -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={}):
Copy link
Contributor

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 define settings=None and then in the function body you call settings = settings or {} which will assign the empty dict if it is None.

Copy link
Author

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!

"""Parses the stdout content of a Quantum ESPRESSO `pw.x` calculation.

:param stdout: the stdout content as a string
Expand Down Expand Up @@ -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 setttings

# Find some useful quantities.
if not parsed_xml.get('number_of_bands', None):
try:
Expand Down Expand Up @@ -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
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
6 changes: 3 additions & 3 deletions aiida_quantumespresso/parsers/pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def parse(self, **kwargs):

parameters = self.node.inputs.parameters.get_dict()
parsed_xml, logs_xml = self.parse_xml(dir_with_bands, parser_options)
parsed_stdout, logs_stdout = self.parse_stdout(parameters, parser_options, parsed_xml)
parsed_stdout, logs_stdout = self.parse_stdout(parameters, parser_options, parsed_xml, settings)

parsed_bands = parsed_stdout.pop('bands', {})
parsed_structure = parsed_stdout.pop('structure', {})
Expand Down Expand Up @@ -295,7 +295,7 @@ def parse_xml(self, dir_with_bands=None, parser_options=None):

return parsed_data, logs

def parse_stdout(self, parameters, parser_options=None, parsed_xml=None):
def parse_stdout(self, parameters, parser_options=None, parsed_xml=None, settings={}):
sudarshanv01 marked this conversation as resolved.
Show resolved Hide resolved
"""Parse the stdout output file.

:param parameters: the input parameters dictionary
Expand All @@ -321,7 +321,7 @@ def parse_stdout(self, parameters, parser_options=None, parsed_xml=None):
return parsed_data, logs

try:
parsed_data, logs = parse_stdout(stdout, parameters, parser_options, parsed_xml)
parsed_data, logs = parse_stdout(stdout, parameters, parser_options, parsed_xml, settings)
except Exception:
logs.critical.append(traceback.format_exc())
self.exit_code_stdout = self.exit_codes.ERROR_UNEXPECTED_PARSER_EXCEPTION
Expand Down
127 changes: 127 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,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)
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
36 changes: 35 additions & 1 deletion tests/calculations/test_pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]]

Expand Down Expand Up @@ -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)
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()
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')
27 changes: 27 additions & 0 deletions tests/calculations/test_pw/test_environ_namelists.aiida.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
11 changes: 11 additions & 0 deletions tests/calculations/test_pw/test_environ_namelists.environ.in
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'
/
17 changes: 15 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
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.

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

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

What part of the code will fail if there is no vacuum?

Copy link
Author

@sudarshanv01 sudarshanv01 Nov 23, 2021

Choose a reason for hiding this comment

The 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 structure_id to be passed to the generate_structure function - let me know if you want this removed too).

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
Expand Down
Loading