`LammpsBaseCalculation`: Add the `files` and `filenames` inputs

[sphuber]

The files input namespace allows a user to pass in additional files to be written to the working directory. By default, the filename to write the content of each SinglefileData node to, defaults to the filename attribute of the node itself. A validator ensures that the list of filenames is unique, because otherwise some files would end up overwriting others.

If this is the case, the optional filenames input can be specified to override the default filename in order to avoid these clashes.

[sphuber]

Only thing missing is docs/example. But wanted to first go through first round of review

[codecov[bot]]

Codecov Report

Merging #78 (7f0dbd6) into develop (b742605) will increase coverage by 0.14%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop      #78      +/-   ##
===========================================
+ Coverage    87.36%   87.50%   +0.14%     
===========================================
  Files           17       17              
  Lines         1369     1385      +16     
===========================================
+ Hits          1196     1212      +16     
  Misses         173      173              

Flag	Coverage Δ
pytests	87.50% <100.00%> (+0.14%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida_lammps/calculations/base.py	97.51% <100.00%> (+0.27%)	:arrow_up:

[JPchico]

It looks quite good. But I just wanted to ask what is the use case for this? What kind of files are we talking about here?

[ltalirz]

this would be an example https://raw.githubusercontent.com/lammps/lammps/develop/bench/data.rhodo

indeed, I did not check whether that particular example would already be handled elsewhere?

[sphuber]

For some more context: the file @ltalirz linked is a data file that is required as an input file by a standard LAMMPS benchmark script: https://github.com/lammps/lammps/blob/develop/bench/in.rhodo

As you can see, it is defined in the input script to instruct LAMMPS to read it:

# Rhodopsin model

units           real
neigh_modify    delay 5 every 1

atom_style      full
bond_style      harmonic
angle_style     charmm
dihedral_style  charmm
improper_style  harmonic
pair_style      lj/charmm/coul/long 8.0 10.0
pair_modify     mix arithmetic
kspace_style    pppm 1e-4

read_data       data.rhodo

fix             1 all shake 0.0001 5 0 m 1.0 a 232
fix             2 all npt temp 300.0 300.0 100.0 &
                z 0.0 0.0 1000.0 mtk no pchain 0 tchain 1

special_bonds   charmm

thermo          50
thermo_style    multi
timestep        2.0

run             100

[sphuber]

If you agree this is a useful use case @JPchico , I can actually add this very script as the example. Given that it comes from the actual LAMMPS repo, it seems to make a lot of sense to support this

[JPchico]

If you agree this is a useful use case @JPchico , I can actually add this very script as the example. Given that it comes from the actual LAMMPS repo, it seems to make a lot of sense to support this

Looking at this it seems basically that the idea is to add files that can be read via the read_data command right?

This is already used when reading the structure via the parameters, so we just need to make sure that if we add this the options do not cannibalize each other.

[ltalirz]

I'm not a lammps expert - could someone perhaps let me know what kind of input data lammps does support? I think an aiida-lammps plugin should not restrict what lammps can do, and it should either support all possible input data in a dedicated way or have a way for users to simply add files that are not natively supported.

This is about practical usability of the plugin - I currently cannot use the plugin to run basic lammps benchmarks

[JPchico]

I'm also not a lammps expert, but I agree that one should be able to do basically everything that lammps allows inside the plugin. The refactoring has helped, but there are things that are still not possible. The script option has solved some of the issues, it would be ideal not to have to rely on it too much, but for now I do not see any simpler solutions.

The pymatgen io from lammps has some nice ideas with stages, but not sure if that is a direction we should follow.

[chrisjsewell]

I think an aiida-lammps plugin should not restrict what lammps can do, and it should either support all possible input data in a dedicated way or have a way for users to simply add files that are not natively supported.

You could argue this of aiida-quantumespresso or basically any calculation plugin that creates an input file(s) from a set of input node types (StructureData + KPoints + ...): "it doesn't support 100% of the possible input parameters, so let's simply replace all the input nodes by a pre-written input file (i.e. SinglefileData)"

This is fine for "quick and dirty", but in the long run defeats the benefits of aiida's modular provenance graph, means you it can't be used to compose workchains, and is not the intended use case for LammpsBaseCalculation

It should be a different calcjob subclass, which is exactly what I did years ago with crystal17.basic and crystal17.main: https://aiida-crystal17.readthedocs.io/en/latest/notebooks/calc_basic.html

[ltalirz]

For practical purposes, the aiida-qe plugin pretty much covers every possible input you can create.

I realize this is much harder in the case of lammps, where the input is a script written in a domain-specific imperative programming language

In any case, I agree with Chris's suggestion to have the "generic" way of running lammps in a separate CalcJob class (this is what I ended up doing in the case of aiida-nwchem as well).

[chrisjsewell]

I agree with Chris's suggestion to have the "generic" way of running lammps in a separate CalcJob class

yep exactly 👍

and then for your particular use case, it would be ideal to identify exactly that aspects that are blocking you from using the current implementation

[JPchico]

Looking at this specific example, there are a couple of things that one would need to add to make sure that it works.

1. The keywords kspace_style, special_bonds, improper_style, dihedral_style, angle_style, bond_style need to be added. Maybe inside the structure block? What do you think @chrisjsewell ? It should be a matter of allowing them in the validation schema, and then adding them in the proper places in the input generation routine.
2. Add a SinglefileData that is what the read_data gets. I think this should be called something like input_simulation_data or some other similar name. One would need to then add a condition that if this is file is provided it is incompatible with a StructureData being given, and then appropriately control the printing.

This should allow the example to be run. Of course, it is a bit harder than using the script directly but it would make everything more easily controllable.

Then of course one should probably add a CalcJob which handles everything in a raw way, where we can give the script and the required raw files to run the script.

[sphuber]

Ok, I see the point of mixing the "raw" and the "orm" interfaces as making things unnecessary complex. How about I rewrite this PR and instead introduce a new plugin LammpsRawCalculation that has the script, files and filenames inputs. I propose we then also remove the script input from the LammpsBaseCalculation that I added, since that hasn't been released anyway. This way, we can just properly document that people wanting full flexibility can use LammpsRawCalculation and for more queryability and integration with other plugins we advise to use LammpsBaseCalculation. Make sense?

[ltalirz]

Cheers, that works for me

[JPchico]

@sphuber I guess that this PR can be closed right? as it has been made obsolete by #80

[sphuber]

Yes, this has been superseded by #80