aiidaplugins/aiida-lammps

LAMMPS plugin for AiiDA

aiida-lammps's Issues

Change the location of the source

The source of the package should be changed to use the src/aiida_lammps style like what is done in aiida-qe

Naming of `CalcJob` plugins

Currently, the calcjob plugins do not contains Lammps in the class name. This is not a problem per se, but since the process_label attribute is taken from the class name, it currently is not very clear. The verdi process list output for example now contains ForceCalculation, MdCalculation, etc. It is not immediately clear that these are LAMMPS calculations.

Ideally, I think these are called LammpsForceCalculation, LammpsMdCalculation, etc.

On this note, I think that BaseLammpsCalculation would better be named LammpsBaseCalculation for consistency. It would also distinguish it from the aiida_lammps.calculations.lammps.BaseLammpsCalculation. I was quite confused to see the same class name for different implementations.

Of course this would be backwards-incompatible if users are importing these directly, but given that they are probably using the entry points, this shouldn't be too much of a problem.

Allow for other `pair_style`

Hello everyone,
thanks a lot first of all for the great plugin. I just wanted to raise the following limitation. Suppose one wants to use other "custom" pair styles, e.g. from patched version using neural network potentials, Allegro for instance, than the strict check from the defaults wouldn't allow for using such potentials.

For my future use-cases, I have the following in mind that patch LAMMPS with custom potentials:

• mir-flare
• Allegro
• Nequip

It would be great to either re-think to the validators, or maybe simply add these ones, as I would like to use them, also in conjunction with other codes I am developing.

Expand the potential class

Right now the potential from LAMMPS supports inputs such as eam, tersoff, lenard-jones and reaxff. Each of these potentials is in principle a distinct data-type. The question is on whether one should condense this as much as possible, so that there is only one potential data type.

If well the LAMMPS potentials can look very different depending on how they were generated, from the data perspective this should not matter, the user should be able to store whatever potential they want, and then use it in their calculations with minimal issues for the plugin, as the large majority of potentials are files which are read by LAMMPS at run time (exception being the lenard-jones potentials)

Hence, what could be done, is to have the potentials being an instance of orm.SingleFileData where the potential is read, from the file. The user would have to indicate which kind of pair_style is being stored, so that the attributes are properly set, similarly to what is done now. This would allow the user the capability of using any kind of potential without having to develop a new plugin for that specific type of potential. Of course a check would have to be done to ensure that the potential lines where LAMMPS knows how to read the potential data are accurate for all (or at least many) of the potential types. If a pair_style has not been checked this could raise a warning letting the user know that this can be an issue. Of course this is a bit more complicated for hybrid potentials.

One issue that also needs to be addressed is how to tag the potentials, in DFT codes one usually has the potential family concept, allowing one to group all the potentials belonging to a given exchange correlation potential or pseudopotential family. This makes reusing and querying for potentials quite simple. However, this is not so for LAMMPS potentials, where tagging is mostly an issue of using the extras provided by the user.

Hence, it might be good to provide a way for the user to set certain variables at the potential creation time to allows for a consistent potential storage and tagging. This could be done by the user passing a dictionary at creation time that will check for certain keys and attach them to the appropriate attributes. I think that a good idea would be to use a similar system than what is used in OpenKIM, where could pass the authors of the potential, the year, references, etc. Of this way one could have a consistent way of querying and storing the potentials. Of course, one should not be prevented from creating the potential node if these variables are not passed, but then it would be up to the user to find the "correct" potential using some sort of convention.

What do you think about this @chrisjsewell ?

dependabot tests failing for python>3.8

For some reason the jsonschema for python > 3.8 is failing, the reason seems that the jsonschema is detecting a conflict in the configuration.

E           jsonschema.exceptions.SchemaError: [{'description': 'strain rates in the x direction', 'type': 'number'}, {'description': 'strain rates in the y direction', 'type': 'number'}] is not of type 'object', 'boolean'
E           
E           Failed validating 'type' in metaschema['allOf'][1]['properties']['properties']['additionalProperties']['$dynamicRef']['allOf'][1]['properties']['properties']['additionalProperties']['$dynamicRef']['allOf'][1]['properties']['properties']['additionalProperties']['$dynamicRef']['allOf'][1]['properties']['properties']['additionalProperties']['$dynamicRef']['allOf'][1]['properties']['items']['$dynamicRef']['allOf'][0]:
``

Make release 1.0.0

Make the release 1.0.0 for aiida-lammps.

Support for dynaphopy

The current dynaphopy calculation is based on a quite old version of aiida-phonopy, before quite large refactoring works were performed. Currently the calculations that depend on this will not work due to these changes. If one pins the repository to the working version of aiida-phonopy one is quite behind in the aiida-core development, i.e not compatible with aiida-core 2.x.

What is probably best is to focus on the current refactoring, make aiida-lammps work in a more flexible manner, and being compatible with aiida-core 2.x and then focus on re-adding the dynaphopy capabilities.

problem with dimensionality for non-periodic boundary conditions in lammps.base

Hi everyone,
in write_structure_block function the get_dimensionality method from AiiDA's StructureData class is used to compute dimensionality and write Lammps input files. The problem is that get_dimensionality actually computes the number of periodic boundary conditions which is not Lammps' dimensionality if I understand correctly.

For example the following structure leads to a Lammps error :

cell = [[1,0,0],
        [0,1,0],
        [0,0,1]]
structure = orm.StructureData(cell = cell, pbc = [True,True,False])

because write_structure_block writes in the Lammps input file :

dimension 2
boundary p p f

instead of :

dimension 3
boundary p p f

Adding group support

One nice feature of LAMMPS is the capability of treating different atoms in a different way via the definitions of groups.

Currently this is not possible in aiida-lammps. Adding groups would be a very good addition to bring more flexibility to the kind of calculations that can be done.

Part of this is addressed by the flexibilization of the input #27 . However, one would need to be able to pass which atoms belong to which group, one way would be via the kind_names in the StructureData where one could encode if an atom belongs to a given group or to another. One could also pass this as a separate Dict, or ArrayData.

The capability of having groups also means that fixes and computes need to be passed by group, meaning that this is something that also has to be dealt with in #27.

Illegal fix_modify command for `fix_modify qeq energy yes`

For conda-forge/osx-64::lammps-2019.06.05-py36_openmpi_5 and later,
an error is raised: ERROR: Illegal fix_modify command (src/fix.cpp:147)

(posted question to [email protected]
'Using qeq/reax fix_modify energy in recent versions of LAMMPS')

Add documentation

The plugin right now has no documentation. A simple documentation should be made and possibly deployed to readthedocs so that users can easily start using the plugin.

dump keyword

How can I change dump keyword?
Now dump_variables is

 dump_variables = "element x y z fx fy fz" 

if atom_style isn't 'charge' in calculations/lammps/forces.py
I would like to use "id xu yu zu fx fy fz".

Transfer to aiidaplugins?

Hey @abelcarreras I have just created https://github.com/aiidaplugins as a central option for people to host their aiida plugin repositories, and invited you as a member.
Would you be willing to transfer this repository to there? (naturally you would still be given administrator access)

How stringent should the schema be

The plan is for the new schema to allow any compute and fix implemented in LAMMPS, whilst placing only very modest checks to ensure that one does not try to run a mdand minimize at the same time. Also one checks that the fixes used in minimize are those that are supported by LAMMPS.

However, should one check that each of the options (when strings are present) are valid LAMMPS inputs, e.g if one sets the calculation of entropy/atom the keywords can be avg and local each one of them accepting a yesor no value, should the schema check for this? and fails if one passes a variable like ave instead of avg?

Return non zero exit code if LAMMPS errors out in the raw calculation

I noticed that in the LammpsRawCalculation the calculation can end up with a zero exit code even if LAMMPS exited with an error. I think that one should add a simple check like the one in the LammpsBaseCalculation where one checks if an error was printed to the stdout and then exit with a non-zero exit code.

aiida-lammps/aiida_lammps/parsers/base.py

Lines 72 to 77 in 77f4594

	if parsed_data["global"]["errors"]:
	for entry in parsed_data["global"]["errors"]:
	self.logger.error(f"LAMMPS emitted the error {entry}")
	return self.exit_codes.ERROR_PARSER_DECTECTED_LAMMPS_RUN_ERROR.format(
	error=entry
	)

Add possibility to run any LAMMPS script

The current CalcJob plugins are quite opinionated in that they all require the structure, potentials and input parameters to be defined through a StructureData, EmpiricalPotential and Dict node. Although this makes sense when comparing to other plugins in the materials science domain, it is kind of restricting. For example, if I wanted to run this benchmark script (which comes from the official LAMMPS benchmark suite), I am not sure that can be done. The "structure" is a 32'000 atom LJ-fluid. In this case, it doesn't really make sense to force a user to convert that to a StructureData since the script instructs LAMMPS to generate the fluid on the fly in the actual calculation.

I wonder if it would make sense to have a really "base" calculation plugin that simply takes a SinglefileData that contains the script to run. I see two options:

• Create a new dedicated CalcJob implementation
• Update the BaseLammpsCalculation to add the optional input_file input. When specified, the structure, potential and parameters inputs are ignored and no longer required.

I'd be fine with either option, but I think the first might be clearest for the user. I would propose simply naming it LammpsCalculation and giving it the entry point lammps. The other plugins would then be more specific as the already are currently.

One advantage of reusing BaseLammpsCalculation is that you could write a single BaseWorkChain for it, instead of having to write two. But then again, there are already a bunch of plugins for particular tasks, so maybe that ship has sailed anyway.

:books: Docs: Documentation overhaul

The documentation right now is quite barebones and the examples cam be a bit confusing. The idea is to make the documentation easier to understand, cleaner, but with enough depth to explain the key concepts, such as the potential, the different types of calculations, etc.

Any ideas or prefferences about this @chrisjsewell and @sphuber?

Example for running from file missing

The possibility to run using a file directly from lammps was introduced but there is no example or documentation.

@sphuber do you think that you could add an example in the documentation or an example file, just for completeness sake?

Thanks!

setup.json for the AiiDA registry

Hi @abelcarreras,
could you move the kwargs of the call to setup() in setup.py into a setup.json, so that setup.py only contains

#!/usr/bin/env python

from __future__ import absolute_import
from setuptools import setup, find_packages
import json

if __name__ == '__main__':
    # Provide static information in setup.json
    # such that it can be discovered automatically
    with open('setup.json', 'r') as info:
        kwargs = json.load(info)
    setup(
        packages=find_packages(),
        long_description=open('README.md').read(),
        long_description_content_type='text/markdown',
        **kwargs)

and the rest in in the json? See e.g. https://github.com/ltalirz/aiida-zeopp/blob/master/setup.py

Note also you will need to create a new MANIFEST.in file in the root directory with this content:
https://github.com/ltalirz/aiida-zeopp/blob/master/MANIFEST.in

This will allow to have the correct information in the AiiDA registry: https://aiidateam.github.io/aiida-registry/plugins/lammps.html

:sparkles: Add workflows for basic types of runs

In the development branch right now everything is tied up to Calculations a set of WorkChains are then needed to perform more dedicated tasks.

The basic WorkChains needed are the following:

• Basic workchain: basically takes the exact same inputs from the calculation and runs them as a workchain instead. This is mostly as to avoid people using the Calculation and instead using Workchains as intended.
• MD workchain: allows the user to just submit a basic MD run using a Workchain instead of a Calculation with specific inputs
• Relaxation/Optimization workchain: This would be a structural optimization, where the user would be able to select which kind of relaxation it wants to perform. Ideally this is done as close as possible to the DFT workchains as to use the same conventions whenever possible.

In the master branch there is a calculation labelled force which seems to perform a single point calculation to determine the energy, forces, etc. Would this be necessary? Is there any case in which this would be important? Perhaps to construct more complex workchains?

A point that is difficult to see how to tackle right now is the BaseRestartWorkChain in principle one would want a single "master" workchain that just accepts the exact same inputs from the Calculation and based on this performs the run that one wants. The only problem I see is that the restart component could be quite complex, as one would need to see which kind of calculation it is that one is trying to do, and depending on that (MD, optimization) decide which kind of correction needs to be done, e.g. increase the number of iterations, change some convergence parameter, etc.

What do you think @chrisjsewell should we have a single BaseRestartWorkChain which has some logic for the different restart cases, and/or should we have dedicated BaseRestartWorkChain one for MD and one for Optimization?

These workchains can then be used to build more complex systems, as I think that these are the minimum that one needs.

Allow for dynamically computing vectorial properties

The current parameters for LammpsCalculation doesn't allow to specify vectorial compute properties, such as forces (fx, fy, fz) when computing porperty/atom for instance. This would be great to have instead of the LammpsRawCalculation, which is less appealing when used as part of other automated workflows.

:books: Update README.md

The readme has information from the deprecated features. This needs to be changed so that it reflects the current capabilities.

Removal of deprecated features

Looking at the changes being done to aiida-lammps right now we have two parsers, several potential types and calculations. All of them are basically redundant by the new BaseLammpsCalculation and LammpsPotentialtypes.

I think that we probably should remove them to make sure that the code is more streamlined and easy to maintain.

The idea was to keep them for backwards compatibility, but in principle for any new calculation one can use the new functions.

Perhaps one can give a way of importing old calculations using a immigrant function that uses the clever input_script input from @sphuber

I'm all for cleaning this up and keeping only one parser, one calculation and one potential structure. The rest of the specific cases can be treated by custom workchains.

What do you think @chrisjsewell ?

Examples and README out of date

I was trying the lammps plugin after the syncing to make it compatible to AiiDA 1.4 and I noticed that the examples and README.md are out of date. Namely they refer to the need to pass the structure when defining the lammps.potential, this seems to have been removed in 71fa4c4

I can make a PR and fix these issues if that is okay.

setup requirements hardcode aiida-core version

The current setup requirements pin the aiida-core version:
https://github.com/abelcarreras/aiida-lammps/blob/8660201047984c8a28efbffaadb2b71c86ba19fc/setup.json#L10

Please replace this with something like aiida-core>=1.0.0b3

Add an explicit way to get extra retrieved files in the calculations

As far as I know only the files explicitly stated in the retrieve_list are copied to the repository. However, specially for the raw calculation one might want to get several more files (trajectories, etc) which are not gotten by default. Thus, adding an entry in the settings where such a list of files is given would help with such an issue.

@sphuber do you agree? Or is there another method of doing this I'm unaware of?

Change linter to ruff+mypy

Ruff is considerably faster, and the inclusion of mypy should help to make sure that everything is type safe. Something that can help with the code quality and avoid unintentional bugs.

:sparkles: Multiple potentials for a calculation

In the current implementation in the develop branch only one potential can be given for a calculation.

This limits the flexibility of what can be done with the plugin. However, applying a "potential family" like is done in the DFT codes is not possible due to the number of possible potentials that exist. Hence another solution is needed.

One would need to add a dynamical namespace for the potential so that one allows more than one entry, while also specifying which potential acts over which atom type.

Adding CODECOV token explicitly to avoid issues with the github actions

Apparently in the new github action one needs to explicitly pass a CODECOV token to avoid having an error when trying to get the coverage, even when one is in a public open source repo. To solve this one needs to put in the CODECOV token as an env variable in the CI/CD.

minimize.energy_tolerance: 0

LAMMPS accepts minimize.energy_tolerance = 0.
Ref. https://docs.lammps.org/minimize.html,
but aiida-lammps doesn't. How about accepting it also in aiida-lammps if possible?
Excuse me that I can't follow the code and I'm not sure whether it is supported or not in the 'devel' branch. Is it better to write in 'Discussions' page?

please rename entry points to start with 'lammps.'

your plugin currently exposes a number of entry points that don't follow the convention to start with the name of the package.
See the travis logs of the aiida registry:
https://travis-ci.org/aiidateam/aiida-registry/jobs/591463873#L342-L347

please consider renaming them to start with 'lammps.'

Handling of the restarfile

Right now by default the binary restart file is written at the end of the simulation and uploaded to the database. This is of course not ideal, as the file can be quite large and it might not be necessary in a large variety of cases.

Also this does not help with possible restarts if the calculation fails before the last step is reached, e.g. by reaching the walltime.

To try to solve this issue, a couple of parameters can be added, one whether the restart will be printed or not, and how often it should be printed (this so that it is printed during the simulation).

If these files are chosen to be uploaded then whichever if found to be last, either the one at the end of the simulation, or the last one that is printed before the simulation fails, is taken and stored as a SinglefileData

The possibility to instead use a RemoteData node will be given, where one can instead pass this folder and use the restartfile present there (probably by making a symbolic link to the calculation folder) to run the restart.

What do you think @chrisjsewell ?

Remove unnecessary inputs from the calculation

Since the changes to include the raw parser and raw calculation there are unnecessary inputs being passed to the calculation. These should be removed.

Modify the base calculation to be more flexible

The current calculation only allows a small set and very strict number of inputs and types of calculations. Instead all these types of calculations should be merged into a single base calculation, which would allow the user to make (as much as possible) any single stage (no actual logical operations and loops in the calculation) LAMMPS calculation.

Modification of structure to work for Lammps

Lammps seems to require that the structure be written in a specific way as to function. Right now this is taken care in the code by the generate_lammps_structure function. Which means that the structure that is actually passed to the run in not exactly the same than the one in the input, it is transformed.

While the transformation is such that the structure is the same by symmetry this can be confusing to the user as the input and output structure would look quite different.

The question is then, is this transformation always needed? Should one transform the output structure so that it matches what one expected from the input (by using the inverse transformation)?

This is tricky since this happens at the Calculation level and hence we cannot call a calcfunction to keep track of what is happening to the structure itself.

dump modify command syntax change

The syntax for dump_modify format has been changed. A new keyword line must be included for input files generation. For example, old format

'dump_modify     aiida format "%16.10f %16.10f %16.10f"\n'

should be updated to

'dump_modify     aiida format line "%16.10f %16.10f %16.10f"\n'

such as in combinate script

Change name of Arrays that does not conform to AiiDA naming convention

The names generated by lammps, might result in array names that are not suitable for aiida. Hence one needs to change the names of the arrays so that they can be stored in the database without problem.

This tends to happen when one is trying to store arrays that have [ in the name.

:recycle: Add warnings and errors to the parsing

Right now the parsing of the lammps output is done via the log file. However, the logfile does not include all the information, as errors and warnings are often not found there. Instead one should use the redirected output from the screen. This output should have all the information of the log file plus the errors and warnings.

The warnings are good to be able to detected minor recoverable problems, and the error is good to be able to detect when and why lammps itself fails.

How to store the atom dependent computes

In the current refactoring approach it is possible for the user to print basically any compute quantity defined by LAMMPS if the application has been compiled with those modules present.

Of course, this has a big issue, how do we store all this data? Right now the positions as a function of time are stored in a trajectory data which is the natural data structure for this data types. However, for the other computes, the trajectory data is not the ideal type, we could of course store each one of these computes as XYData, or just dump everything in an ArrayData. However, this would make getting step based information quite difficult.

I wonder if there is a better data type or if it would be a good idea to make a derived data type, perhaps from the ArrayData with custom methods to get the step wise information.

What do you think @chrisjsewell ?

Add tests for the new base calculation

The current base calculation in the develop branch is not currently being tested.

:ok_hand: Expanding the list of required values in the `LammpsPotentialData`

It has been brought to my attention that the author of the potential, the year and title should be required entries to ensure that the potential is easier to track. These keywords should be set in the required attributes of the LammpsPotentialData type.

aiida_lammps plugin

I tried to install the plugin using AiiDa v.1 and received following error message "Could not find a version that satisfies the requirement aiida-lammps.1 (from versions: none". Is lammps plugin ready to be used?

Simplify implementation by separating coulombic interactions into own term

There are a huge number of compound pair-styles, e.g. buck, buck/coul/cut, buck/coul/long, buck/long/coul/long, which makes implementing all such styles a massive endeavour.

I propose simplifying this by disallowing coulombic terms to be compounded with n-body interactions where it is possible to equivalently do so with multiple simpler potentials.

For example, an example input for a Buckingham potential with coulombic interactions and long range interactions:

pair_style buck/coul/long 12
pair_coeff * * 0 1 0
pair_coeff 1 1 1000 0.1 10
pair_coeff 1 2 2000 0.2 20
pair_coeff 2 2 3000 0.3 30

is equivalent to:

pair_style coul/long 12
pair_coeff * *

pair_style buck
pair_coeff 1 1 1000 0.1 10
pair_coeff 1 2 2000 0.2 20
pair_coeff 2 2 3000 0.3 30

By separating the terms, we are left with a manageable number of modifiers both for coulombic and non-coulombic potential forms to be implemented.

Preparation for aiida 2.x compatibility

In the develop branch most of the things should be compatible with aiida-2.x. But it needs to be thoroughly tested and make sure that no outstanding deprecation warnings are left. The idea of this issue is to act as a placeholder to ensure that we can discuss if anything needs to be done for this.

LammpsTrajectories not working with aiida-core 1.65

It seems to be that it is not possible to store the zip files in the archive as it was previously done when using aiida-core 1.64

The problem seems to be that in create_file_from_filelike one must pass a string instead of a bytes object, at least this is what the traceback seems to indicate when one tries to store the object.

aiida_lammps/tests/test_trajectory.py:90: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
aiida_lammps/data/trajectory.py:50: in __init__
    self.set_from_fileobj(handle.readlines(), aliases)
aiida_lammps/data/trajectory.py:127: in set_from_fileobj
    self.put_object_from_filelike(
/usr/share/miniconda/lib/python3.9/site-packages/aiida/orm/nodes/node.py:711: in put_object_from_filelike
    self._repository.put_object_from_filelike(handle, path, mode, encoding, force)
/usr/share/miniconda/lib/python3.9/site-packages/aiida/orm/utils/_repository.py:231: in put_object_from_filelike
    folder.create_file_from_filelike(handle, key, mode=mode, encoding=encoding)
/usr/share/miniconda/lib/python3.9/site-packages/aiida/common/folders.py:228: in create_file_from_filelike
    shutil.copyfileobj(filelike, handle)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

fsrc = <tempfile._TemporaryFileWrapper object at 0x7f11e2252550>
fdst = <_io.TextIOWrapper name='/tmp/tmpui8y2zpd/test_repo/sandbox/tmpkqa5ijui/path/trajectory.zip' mode='w' encoding='utf8'>
length = 65536

    def copyfileobj(fsrc, fdst, length=0):
        """copy data from file-like object fsrc to file-like object fdst"""
        # Localize variable access to minimize overhead.
        if not length:
            length = COPY_BUFSIZE
        fsrc_read = fsrc.read
        fdst_write = fdst.write
        while True:
            buf = fsrc_read(length)
            if not buf:
                break
>           fdst_write(buf)
E           TypeError: write() argument must be str, not bytes

/usr/share/miniconda/lib/python3.9/shutil.py:208: TypeError

Modify the inputfile creation to make it more flexible

Change the generation of the input file so that it is dynamic, i.e. depending on which variables are passed the input file will be filled. Of this way the input file would be generated by a separate function for all Calculations and WorkChains instead of being written in each Calculation as it is now.